From c92fc25a9bf8a5b11123eb47157821818e2cad7f Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 15:54:57 -0600 Subject: [PATCH 01/26] Phase 1: Constants and ToolUsageReport model fields Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/constants.py | 26 ++++++++ blockscout_mcp_server/models.py | 20 ++++++ tests/test_tool_usage_report.py | 104 +++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 tests/test_tool_usage_report.py diff --git a/blockscout_mcp_server/constants.py b/blockscout_mcp_server/constants.py index a4aca0ab..348c09bc 100644 --- a/blockscout_mcp_server/constants.py +++ b/blockscout_mcp_server/constants.py @@ -1,6 +1,8 @@ # SPDX-License-Identifier: LicenseRef-Blockscout """Constants used throughout the Blockscout MCP Server.""" +from typing import Literal + from blockscout_mcp_server import __version__ SERVER_VERSION = __version__ @@ -104,3 +106,27 @@ # The maximum length for a transaction's input data field before it's truncated. # 514 = '0x' prefix + 512 hex characters (256 bytes). INPUT_DATA_TRUNCATION_LIMIT = 514 + +# Versioned domain-separation prefix for the PRO API key fingerprint hash. The "v1" is +# deliberate: it lets the hashing scheme be versioned later without silently colliding +# with old fingerprints. It is not a secret and provides domain separation, not +# brute-force resistance (PRO API keys are high-entropy, so preimage attacks are +# infeasible; a server-side HMAC pepper is deferred to a follow-up change). +PRO_API_KEY_HASH_PREFIX = "bs-pro-key-v1:" + +# The three real authorization origins a key can come from, as computed by the +# ctx-derived helpers. Defined as a single Literal alias (not separate string +# constants) because a Literal[...] annotation cannot be built from string-constant +# variables; the model field and the helper return type both import this alias. +AuthOrigin = Literal["client", "server", "none"] + +# Legacy sentinel used only at the Mixpanel layer as the default for community +# reports that predate the `auth_origin` field. Deliberately not part of `AuthOrigin`: +# it is never a valid report value and never returned by the helpers. +AUTH_ORIGIN_UNKNOWN = "unknown" + +# A SHA-256 hex digest is always exactly 64 characters. This is the defensive +# max_length cap on the inbound community report's `api_key_fingerprint` field +# (mirroring the rationale documented for `_MAX_KEY_LENGTH` in pro_api_key_context.py); +# the field's `pattern` enforces the full exact shape. +PRO_API_KEY_FINGERPRINT_MAX_LENGTH = 64 diff --git a/blockscout_mcp_server/models.py b/blockscout_mcp_server/models.py index 8cef6239..b067cd41 100644 --- a/blockscout_mcp_server/models.py +++ b/blockscout_mcp_server/models.py @@ -5,6 +5,8 @@ from pydantic import BaseModel, ConfigDict, Field +from blockscout_mcp_server.constants import PRO_API_KEY_FINGERPRINT_MAX_LENGTH, AuthOrigin + # --- Generic Type Variable --- T = TypeVar("T") @@ -15,6 +17,24 @@ class ToolUsageReport(BaseModel): client_name: str client_version: str protocol_version: str + auth_origin: AuthOrigin | None = Field( + default=None, + description=( + "The origin of the authorization used for the reported call: 'client' for a " + "client-supplied PRO API key, 'server' for a server-configured key, or 'none' for " + "no usable key. Absent on legacy payloads that predate this field." + ), + ) + api_key_fingerprint: str | None = Field( + default=None, + pattern=r"^[0-9a-f]{64}$", + max_length=PRO_API_KEY_FINGERPRINT_MAX_LENGTH, + description=( + "A one-way, non-reversible SHA-256 hex digest fingerprint of the effective PRO API " + "key used for the reported call, or null if no key was used. This field is accepted " + "over the wire but is not yet consumed (not forwarded to Mixpanel, not persisted)." + ), + ) # --- Models for Pagination --- diff --git a/tests/test_tool_usage_report.py b/tests/test_tool_usage_report.py new file mode 100644 index 00000000..d04dc17a --- /dev/null +++ b/tests/test_tool_usage_report.py @@ -0,0 +1,104 @@ +# SPDX-License-Identifier: LicenseRef-Blockscout +"""Tests for the `ToolUsageReport` Pydantic model's optional auth-signal fields.""" + +import pytest +from pydantic import ValidationError + +from blockscout_mcp_server.models import ToolUsageReport + +VALID_FINGERPRINT = "a" * 64 + + +def _base_payload(**overrides): + payload = { + "tool_name": "get_block_info", + "tool_args": {"chain_id": "1"}, + "client_name": "test-client", + "client_version": "1.0.0", + "protocol_version": "2024-11-05", + } + payload.update(overrides) + return payload + + +def test_legacy_payload_defaults_new_fields_to_none(): + """A payload with only the five original fields validates, and both new fields default to None.""" + report = ToolUsageReport(**_base_payload()) + + assert report.auth_origin is None + assert report.api_key_fingerprint is None + + +@pytest.mark.parametrize("origin", ["client", "server", "none"]) +def test_auth_origin_round_trips_each_valid_value(origin): + """A payload supplying auth_origin as each of client/server/none validates and round-trips it.""" + report = ToolUsageReport(**_base_payload(auth_origin=origin)) + + assert report.auth_origin == origin + + +def test_auth_origin_rejects_out_of_enum_value(): + """A payload supplying an out-of-enum auth_origin raises a pydantic ValidationError.""" + with pytest.raises(ValidationError): + ToolUsageReport(**_base_payload(auth_origin="bogus")) + + +def test_api_key_fingerprint_round_trips_valid_value(): + """A payload supplying a valid 64-char lowercase hex api_key_fingerprint validates and round-trips.""" + report = ToolUsageReport(**_base_payload(api_key_fingerprint=VALID_FINGERPRINT)) + + assert report.api_key_fingerprint == VALID_FINGERPRINT + + +def test_explicit_none_auth_origin_string_with_null_fingerprint_round_trips(): + """Explicitly setting auth_origin="none" and api_key_fingerprint=None round-trips None. + + This is the no-key wire shape an updated reporter actually sends (Phase 4 includes the key + unconditionally), distinct from omitting the fields. It guards against the `pattern` + constraint accidentally rejecting an explicit `null`. + """ + report = ToolUsageReport(**_base_payload(auth_origin="none", api_key_fingerprint=None)) + + assert report.auth_origin == "none" + assert report.api_key_fingerprint is None + + +def test_explicit_null_auth_origin_with_null_fingerprint_round_trips(): + """Explicitly setting auth_origin=None (Python None / JSON null) round-trips both fields as None. + + This pins the field's typing to `AuthOrigin | None`. A bare `Literal[...]` that merely + defaults to None would accept an omitted field but reject an explicit null; only this test + catches that, since the omitted and string-"none" cases pass either way. + """ + report = ToolUsageReport(**_base_payload(auth_origin=None, api_key_fingerprint=None)) + + assert report.auth_origin is None + assert report.api_key_fingerprint is None + + +def test_api_key_fingerprint_rejects_too_short_value(): + """A malformed (too short) api_key_fingerprint raises a pydantic ValidationError.""" + with pytest.raises(ValidationError): + ToolUsageReport(**_base_payload(api_key_fingerprint="abc")) + + +def test_api_key_fingerprint_rejects_non_hex_value(): + """A 64-character but non-hex api_key_fingerprint raises a pydantic ValidationError.""" + with pytest.raises(ValidationError): + ToolUsageReport(**_base_payload(api_key_fingerprint="z" * 64)) + + +def test_api_key_fingerprint_rejects_uppercase_hex_value(): + """A 64-character uppercase-hex api_key_fingerprint raises a pydantic ValidationError. + + The pattern `^[0-9a-f]{64}$` only accepts lowercase hex, matching the lowercase output of + `hashlib.sha256(...).hexdigest()`. + """ + with pytest.raises(ValidationError): + ToolUsageReport(**_base_payload(api_key_fingerprint="A" * 64)) + + +def test_api_key_fingerprint_rejects_over_length_value(): + """An over-length api_key_fingerprint raises a pydantic ValidationError.""" + with pytest.raises(ValidationError): + ToolUsageReport(**_base_payload(api_key_fingerprint="a" * 65)) From 2c01f004dab3fc7bc0a35e4bd6b417fb1fb9b99f Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 15:58:51 -0600 Subject: [PATCH 02/26] Phase 2: ctx-derived auth-origin and fingerprint helpers Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/pro_api_key_context.py | 78 +++++++ .../test_pro_api_key_context_auth_signals.py | 212 ++++++++++++++++++ 2 files changed, 290 insertions(+) create mode 100644 tests/test_pro_api_key_context_auth_signals.py diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index fc60ad6b..ca92897f 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -48,6 +48,7 @@ from __future__ import annotations import functools +import hashlib import inspect import logging import math @@ -58,6 +59,7 @@ from blockscout_mcp_server.client_meta import get_header_case_insensitive from blockscout_mcp_server.config import config +from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX, AuthOrigin logger = logging.getLogger(__name__) @@ -231,6 +233,82 @@ def extract_client_pro_api_key_from_ctx(ctx: Any) -> ClientKeyState: return _ABSENT +# --------------------------------------------------------------------------- +# 4b. ctx-derived auth-origin and fingerprint helpers (for analytics/telemetry) +# --------------------------------------------------------------------------- +# +# Analytics and telemetry (e.g. log_tool_invocation / community reporting) run +# *outside* @pro_api_key_scope, so they cannot read the _client_key_state +# ContextVar (see the @pro_api_key_scope docstring above). These helpers derive +# the same client-key → server-key precedence signal directly from ctx headers +# instead, by reusing extract_client_pro_api_key_from_ctx(). They must NOT call +# resolve_pro_api_key(): it reads the request-scoped ContextVar (unset on this +# path) and raises on a malformed key, whereas these helpers must never raise. + + +def _fingerprint_pro_api_key(key: str) -> str: + """Return the hex SHA-256 digest of *key*, domain-separated by ``PRO_API_KEY_HASH_PREFIX``. + + ``hashlib.sha256`` takes a bytes-like object, not a ``str``; the + prefix+encoding scheme is centralized here so both the client-key and + server-key branches of :func:`compute_api_key_fingerprint` share one + construction. The raw key is never returned, logged, or placed anywhere + but this hash input. + """ + return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 + + +def compute_auth_origin(ctx: Any) -> AuthOrigin: + """Derive the authorization origin for *ctx* directly from request headers. + + Never raises (delegates to :func:`extract_client_pro_api_key_from_ctx`, + which never raises) and never calls :func:`resolve_pro_api_key`. + + - Valid client key → ``"client"``. + - Malformed client key → ``"none"`` (no fallback to the server key for a + malformed submission — the request will fail at the PRO API chokepoint). + - No client key (absent) → ``"server"`` when ``config.pro_api_key`` is + truthy, otherwise ``"none"``. + """ + state = extract_client_pro_api_key_from_ctx(ctx) + + if isinstance(state, _Valid): + return "client" + + if isinstance(state, _Malformed): + return "none" + + # _Absent — fall back to whether a server key is configured. + return "server" if config.pro_api_key else "none" + + +def compute_api_key_fingerprint(ctx: Any) -> str | None: + """Derive a one-way fingerprint of the effective PRO API key for *ctx*. + + Never raises and never calls :func:`resolve_pro_api_key`. Mirrors the + precedence in :func:`compute_auth_origin`: + + - Valid client key → hash of the client value. + - Malformed client key → ``None`` (consistent with ``auth_origin = "none"``). + - Absent → hash of ``config.pro_api_key`` when truthy, otherwise ``None``. + + The raw key is never returned, logged, or used anywhere but as the hash + input. + """ + state = extract_client_pro_api_key_from_ctx(ctx) + + if isinstance(state, _Valid): + return _fingerprint_pro_api_key(state.value) + + if isinstance(state, _Malformed): + return None + + # _Absent — fall back to the configured server key, if any. + if config.pro_api_key: + return _fingerprint_pro_api_key(config.pro_api_key) + return None + + # --------------------------------------------------------------------------- # 5. Resolver — applies client-key → server-key precedence # --------------------------------------------------------------------------- diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py new file mode 100644 index 00000000..d055afd7 --- /dev/null +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -0,0 +1,212 @@ +# SPDX-License-Identifier: LicenseRef-Blockscout +"""Unit tests for the ctx-derived auth-origin and fingerprint helpers. + +Kept in a focused sibling module rather than grown into +``tests/test_pro_api_key_context.py`` (already 466 LOC, close to the rule +``210`` 500-LOC limit) per the rule ``210`` guidance. +""" + +from __future__ import annotations + +import hashlib +from types import SimpleNamespace + +from starlette.datastructures import Headers + +from blockscout_mcp_server.config import config +from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX +from blockscout_mcp_server.pro_api_key_context import ( + compute_api_key_fingerprint, + compute_auth_origin, +) + +_HEADER_NAME = "Blockscout-MCP-Pro-Api-Key" + + +def _ctx_with_header(header_name: str, header_value: str) -> SimpleNamespace: + """Build a minimal MCP-like context carrying *header_value* under *header_name*. + + Mirrors the helper in ``tests/test_pro_api_key_context.py``. + """ + headers = Headers(headers={header_name.upper(): header_value}) + request = SimpleNamespace(headers=headers) + return SimpleNamespace(request_context=SimpleNamespace(request=request)) + + +def _ctx_with_malformed_header(header_name: str, header_value: str) -> SimpleNamespace: + """Build a context whose header value contains control characters. + + Uses a plain dict so a value that real ``starlette.datastructures.Headers`` + would refuse to encode can still be injected (mirrors the equivalent helper + usage in ``tests/test_pro_api_key_context.py``). + """ + headers = {header_name: header_value} + request = SimpleNamespace(headers=headers) + return SimpleNamespace(request_context=SimpleNamespace(request=request)) + + +def _expected_fingerprint(key: str) -> str: + """Compute the expected fingerprint via the same explicit UTF-8 byte construction.""" + return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 + + +# =========================================================================== +# compute_auth_origin +# =========================================================================== + + +def test_auth_origin_valid_client_header_is_client(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_auth_origin(ctx) == "client" + + +def test_auth_origin_malformed_header_is_none(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") + assert compute_auth_origin(ctx) == "none" + + +def test_auth_origin_malformed_over_length_header_is_none(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_malformed_header(_HEADER_NAME, "a" * 257) + assert compute_auth_origin(ctx) == "none" + + +def test_auth_origin_absent_header_with_server_key_is_server(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_auth_origin(ctx) == "server" + + +def test_auth_origin_absent_header_with_no_server_key_is_none(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_auth_origin(ctx) == "none" + + +def test_auth_origin_feature_disabled_with_server_key_is_server(monkeypatch): + """Header feature disabled (empty header config) + server key configured -> server. + + "Feature disabled" alone does not force "none" -- a disabled header feature + makes the extractor return _Absent, which falls through to the server-key + branch just like a genuinely absent header. + """ + monkeypatch.setattr(config, "pro_api_key_header", "", raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_auth_origin(ctx) == "server" + + +def test_auth_origin_feature_disabled_with_no_server_key_is_none(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", "", raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_auth_origin(ctx) == "none" + + +# =========================================================================== +# compute_api_key_fingerprint +# =========================================================================== + + +def test_fingerprint_valid_client_header_matches_expected_hash(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_api_key_fingerprint(ctx) == _expected_fingerprint("client-key-123") + + +def test_fingerprint_absent_header_with_server_key_matches_expected_hash(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_api_key_fingerprint(ctx) == _expected_fingerprint("server-key") + + +def test_fingerprint_malformed_header_is_none(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") + assert compute_api_key_fingerprint(ctx) is None + + +def test_fingerprint_absent_header_with_no_server_key_is_none(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_api_key_fingerprint(ctx) is None + + +# =========================================================================== +# No-fallback precedence (competing server key configured) +# =========================================================================== +# +# These cases set config.pro_api_key = "server-key" so a server-fallback +# regression cannot hide: a valid client header must still win, and a +# malformed client header must still yield no usable key (no silent fallback +# to the configured server key). Mirrors the precedence already enforced by +# resolve_pro_api_key(). Kept distinct from the absent-header cases above, +# which deliberately exercise the server branch. + + +def test_auth_origin_valid_client_header_wins_over_configured_server_key(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_auth_origin(ctx) == "client" + + +def test_fingerprint_valid_client_header_wins_over_configured_server_key(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + fingerprint = compute_api_key_fingerprint(ctx) + assert fingerprint == _expected_fingerprint("client-key-123") + assert fingerprint != _expected_fingerprint("server-key") + + +def test_auth_origin_malformed_client_header_does_not_fall_back_to_server_key(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") + assert compute_auth_origin(ctx) == "none" + + +def test_fingerprint_malformed_client_header_does_not_fall_back_to_server_key(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") + assert compute_api_key_fingerprint(ctx) is None + + +# =========================================================================== +# Fingerprint safety: raw key never leaks; prefix actually participates +# =========================================================================== + + +def test_fingerprint_never_equals_or_contains_raw_key(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + raw_key = "super-secret-client-key-456" + ctx = _ctx_with_header(_HEADER_NAME, raw_key) + fingerprint = compute_api_key_fingerprint(ctx) + assert fingerprint is not None + assert fingerprint != raw_key + assert raw_key not in fingerprint + + +def test_fingerprint_prefix_actually_participates(monkeypatch): + """The result must differ from a hash computed without the domain-separation prefix.""" + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + raw_key = "client-key-789" + ctx = _ctx_with_header(_HEADER_NAME, raw_key) + fingerprint = compute_api_key_fingerprint(ctx) + unprefixed_hash = hashlib.sha256(raw_key.encode("utf-8")).hexdigest() + assert fingerprint != unprefixed_hash From 6c3dcbcf5bd1af6d8d73c42b75e849d78b4062d6 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 16:03:07 -0600 Subject: [PATCH 03/26] Phase 3: Direct analytics wiring Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/analytics.py | 5 +- tests/test_analytics.py | 126 ++++++++++++++++++++++++++++- 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/blockscout_mcp_server/analytics.py b/blockscout_mcp_server/analytics.py index af0ba89f..41d0b110 100644 --- a/blockscout_mcp_server/analytics.py +++ b/blockscout_mcp_server/analytics.py @@ -37,8 +37,9 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: # noqa: D401 - simple pl get_header_case_insensitive, ) from blockscout_mcp_server.config import config -from blockscout_mcp_server.constants import RESOURCE_READ_EVENT +from blockscout_mcp_server.constants import AUTH_ORIGIN_UNKNOWN, RESOURCE_READ_EVENT from blockscout_mcp_server.models import ToolUsageReport +from blockscout_mcp_server.pro_api_key_context import compute_auth_origin logger = logging.getLogger(__name__) @@ -225,6 +226,7 @@ def track_tool_invocation( "tool_args": tool_args, "protocol_version": protocol_version, "source": _determine_call_source(ctx), + "auth_origin": compute_auth_origin(ctx), } meta = {"ip": ip} if ip else None @@ -271,6 +273,7 @@ def track_community_usage(report: ToolUsageReport, ip: str, user_agent: str) -> "tool_args": report.tool_args, "protocol_version": report.protocol_version, "source": "community", + "auth_origin": report.auth_origin if report.auth_origin is not None else AUTH_ORIGIN_UNKNOWN, } meta = {"ip": ip} if ip else None diff --git a/tests/test_analytics.py b/tests/test_analytics.py index ec3bffa3..bed9cfaa 100644 --- a/tests/test_analytics.py +++ b/tests/test_analytics.py @@ -145,6 +145,38 @@ def test_tracks_with_intermediary_no_client_or_user_agent(monkeypatch): assert args[2]["client_name"] == "N/A/ClaudeDesktop" +def test_tracks_auth_origin_server_when_no_client_header_and_server_key(monkeypatch): + """No client-supplied key, but a server key is configured -> auth_origin == 'server'.""" + monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) + monkeypatch.setattr(server_config, "pro_api_key", "server-secret", raising=False) + headers = {"x-forwarded-for": "203.0.113.5", "user-agent": "pytest-UA"} + req = DummyRequest(headers=headers) + ctx = DummyCtx(request=req, client_name="clientA", client_version="1.0.0") + with patch("blockscout_mcp_server.analytics.Mixpanel") as mp_cls: + mp_instance = MagicMock() + mp_cls.return_value = mp_instance + analytics.set_http_mode(True) + analytics.track_tool_invocation(ctx, "tool_name", {"x": 2}) + args, _ = mp_instance.track.call_args + assert args[2]["auth_origin"] == "server" + + +def test_tracks_auth_origin_none_when_no_client_header_and_no_server_key(monkeypatch): + """Neither a client-supplied key nor a server key -> auth_origin == 'none'.""" + monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) + monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) + headers = {"x-forwarded-for": "203.0.113.5", "user-agent": "pytest-UA"} + req = DummyRequest(headers=headers) + ctx = DummyCtx(request=req, client_name="clientA", client_version="1.0.0") + with patch("blockscout_mcp_server.analytics.Mixpanel") as mp_cls: + mp_instance = MagicMock() + mp_cls.return_value = mp_instance + analytics.set_http_mode(True) + analytics.track_tool_invocation(ctx, "tool_name", {"x": 2}) + args, _ = mp_instance.track.call_args + assert args[2]["auth_origin"] == "none" + + def test_track_event_tracks_when_enabled(monkeypatch): monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) req = DummyRequest(headers={"user-agent": "pytest-UA"}, host="203.0.113.5") @@ -210,6 +242,10 @@ def test_pro_api_key_not_in_analytics_payload(monkeypatch): assert client_key not in str(properties) assert client_key not in str(kwargs) + # The client supplied a valid header, so auth_origin must be 'client'. + assert properties.get("auth_origin") == "client" + assert "api_key_fingerprint" not in properties + def test_pro_api_key_not_in_analytics_payload_rest_source(monkeypatch): """The client-supplied PRO API key must not appear in the Mixpanel payload on the REST path. @@ -257,6 +293,10 @@ def test_pro_api_key_not_in_analytics_payload_rest_source(monkeypatch): # Confirm the source is correctly reported as 'rest' assert properties.get("source") == "rest" + # The client supplied a valid header, so auth_origin must be 'client'. + assert properties.get("auth_origin") == "client" + assert "api_key_fingerprint" not in properties + def test_track_community_usage(monkeypatch): monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) @@ -283,6 +323,83 @@ def test_track_community_usage(monkeypatch): assert kwargs.get("meta") == {"ip": "203.0.113.5"} +def test_track_community_usage_auth_origin_from_report(monkeypatch): + """auth_origin is forwarded verbatim from the report when present.""" + monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) + with patch("blockscout_mcp_server.analytics.Mixpanel") as mp_cls: + mp_instance = MagicMock() + mp_cls.return_value = mp_instance + analytics.set_http_mode(True) + report = ToolUsageReport( + tool_name="foo", + tool_args={"a": 1}, + client_name="cli", + client_version="1.0", + protocol_version="1.1", + auth_origin="server", + ) + analytics.track_community_usage(report, ip="203.0.113.5", user_agent="ua") + args, _ = mp_instance.track.call_args + properties = args[2] + assert properties["auth_origin"] == "server" + + +def test_track_community_usage_auth_origin_defaults_to_unknown(monkeypatch): + """A legacy report with no auth_origin maps to the 'unknown' sentinel.""" + monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) + with patch("blockscout_mcp_server.analytics.Mixpanel") as mp_cls: + mp_instance = MagicMock() + mp_cls.return_value = mp_instance + analytics.set_http_mode(True) + report = ToolUsageReport( + tool_name="foo", + tool_args={"a": 1}, + client_name="cli", + client_version="1.0", + protocol_version="1.1", + auth_origin=None, + ) + analytics.track_community_usage(report, ip="203.0.113.5", user_agent="ua") + args, _ = mp_instance.track.call_args + properties = args[2] + assert properties["auth_origin"] == "unknown" + + +def test_track_community_usage_fingerprint_never_reaches_mixpanel(monkeypatch): + """The api_key_fingerprint must not leak anywhere in the Mixpanel call. + + Checks the entire call (distinct_id, event name, properties, meta) for the + fingerprint value, not merely the `properties["api_key_fingerprint"]` key + -- this also catches a leak via distinct_id or a differently named property. + Also asserts distinct_id is unaffected by the fingerprint (still derived + only from ip/client_name/client_version), proving identity is not yet + strengthened by it. + """ + monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) + distinctive_fingerprint = "ab" * 32 # 64-char lowercase hex, easy to spot in a leak + with patch("blockscout_mcp_server.analytics.Mixpanel") as mp_cls: + mp_instance = MagicMock() + mp_cls.return_value = mp_instance + analytics.set_http_mode(True) + report = ToolUsageReport( + tool_name="foo", + tool_args={"a": 1}, + client_name="cli", + client_version="1.0", + protocol_version="1.1", + auth_origin="client", + api_key_fingerprint=distinctive_fingerprint, + ) + analytics.track_community_usage(report, ip="203.0.113.5", user_agent="ua") + mp_instance.track.assert_called_once() + call_args = mp_instance.track.call_args + assert distinctive_fingerprint not in str(call_args) + + args, _ = call_args + expected_distinct_id = analytics._build_distinct_id("203.0.113.5", report.client_name, report.client_version) + assert args[0] == expected_distinct_id + + # --------------------------------------------------------------------------- # track_resource_read tests # --------------------------------------------------------------------------- @@ -309,7 +426,11 @@ def test_track_resource_read_emits_correct_event(monkeypatch): """When enabled, mp.track is called with RESOURCE_READ event and uri in tool_args.""" monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) uri = "blockscout-mcp://skill/SKILL.md" - headers = {"x-forwarded-for": "203.0.113.5", "user-agent": "pytest-UA"} + headers = { + "x-forwarded-for": "203.0.113.5", + "user-agent": "pytest-UA", + "Blockscout-MCP-Pro-Api-Key": "client-secret", + } req = DummyRequest(headers=headers) ctx = DummyCtx(request=req, client_name="clientA", client_version="1.0.0") with patch("blockscout_mcp_server.analytics.Mixpanel") as mp_cls: @@ -331,3 +452,6 @@ def test_track_resource_read_emits_correct_event(monkeypatch): assert properties["protocol_version"] == "2024-11-05" assert properties["ip"] == "203.0.113.5" assert "source" in properties + # track_resource_read delegates to track_tool_invocation, so a header-present + # context yields 'client' for auth_origin, inherited automatically. + assert properties["auth_origin"] == "client" From 5f47f1cd531003d29337b144469b2c133f9a9065 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 16:06:27 -0600 Subject: [PATCH 04/26] Phase 4: Community telemetry payload Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/telemetry.py | 23 ++++- tests/test_telemetry.py | 140 +++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 2 deletions(-) diff --git a/blockscout_mcp_server/telemetry.py b/blockscout_mcp_server/telemetry.py index 7053eb20..2ca1aa7b 100644 --- a/blockscout_mcp_server/telemetry.py +++ b/blockscout_mcp_server/telemetry.py @@ -21,8 +21,15 @@ async def send_community_usage_report( client_name: str, client_version: str, protocol_version: str, + auth_origin: str | None = None, + api_key_fingerprint: str | None = None, ) -> None: - """Send a fire-and-forget tool usage report if in community telemetry mode.""" + """Send a fire-and-forget tool usage report if in community telemetry mode. + + ``auth_origin`` and ``api_key_fingerprint`` are already-computed signals (see + :mod:`blockscout_mcp_server.pro_api_key_context`); this function is a dumb conduit + and must never receive or handle a raw API key. + """ if config.disable_community_telemetry: return @@ -37,6 +44,8 @@ async def send_community_usage_report( "client_name": client_name, "client_version": client_version, "protocol_version": protocol_version, + "auth_origin": auth_origin, + "api_key_fingerprint": api_key_fingerprint, } url = f"{COMMUNITY_TELEMETRY_URL}{COMMUNITY_TELEMETRY_ENDPOINT}" @@ -52,10 +61,20 @@ async def send_community_resource_report( client_name: str, client_version: str, protocol_version: str, + auth_origin: str | None = None, + api_key_fingerprint: str | None = None, ) -> None: """Send a fire-and-forget resource read report if in community telemetry mode. Delegates to :func:`send_community_usage_report` using the ``RESOURCE_READ`` event sentinel so all gating and POST logic is reused verbatim. """ - await send_community_usage_report(RESOURCE_READ_EVENT, {"uri": uri}, client_name, client_version, protocol_version) + await send_community_usage_report( + RESOURCE_READ_EVENT, + {"uri": uri}, + client_name, + client_version, + protocol_version, + auth_origin=auth_origin, + api_key_fingerprint=api_key_fingerprint, + ) diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 4a581d13..44c4ad59 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -30,6 +30,8 @@ async def test_send_community_usage_report_sent(monkeypatch): "client_name": "client", "client_version": "1.0", "protocol_version": "1.1", + "auth_origin": None, + "api_key_fingerprint": None, }, headers=ANY, timeout=2.0, @@ -70,6 +72,106 @@ async def test_send_community_usage_report_network_error(monkeypatch): mock_client.post.assert_awaited_once() +@pytest.mark.asyncio +async def test_send_community_usage_report_includes_auth_origin_and_fingerprint(monkeypatch): + """A call passing auth_origin='client' and a fingerprint string includes both verbatim.""" + monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) + monkeypatch.setattr(config, "mixpanel_token", "", raising=False) + fingerprint = "a" * 64 + mock_client = AsyncMock() + mock_ctx_mgr = AsyncMock() + mock_ctx_mgr.__aenter__.return_value = mock_client + with patch("httpx.AsyncClient", return_value=mock_ctx_mgr): + await telemetry.send_community_usage_report( + "tool", + {"a": 1}, + "client", + "1.0", + "1.1", + auth_origin="client", + api_key_fingerprint=fingerprint, + ) + url = f"{COMMUNITY_TELEMETRY_URL}{COMMUNITY_TELEMETRY_ENDPOINT}" + mock_client.post.assert_awaited_once_with( + url, + json={ + "tool_name": "tool", + "tool_args": {"a": 1}, + "client_name": "client", + "client_version": "1.0", + "protocol_version": "1.1", + "auth_origin": "client", + "api_key_fingerprint": fingerprint, + }, + headers=ANY, + timeout=2.0, + ) + + +@pytest.mark.asyncio +async def test_send_community_usage_report_none_origin_serializes_null_fingerprint(monkeypatch): + """A call passing auth_origin='none' and api_key_fingerprint=None serializes the fingerprint as JSON null.""" + monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) + monkeypatch.setattr(config, "mixpanel_token", "", raising=False) + mock_client = AsyncMock() + mock_ctx_mgr = AsyncMock() + mock_ctx_mgr.__aenter__.return_value = mock_client + with patch("httpx.AsyncClient", return_value=mock_ctx_mgr): + await telemetry.send_community_usage_report( + "tool", + {"a": 1}, + "client", + "1.0", + "1.1", + auth_origin="none", + api_key_fingerprint=None, + ) + url = f"{COMMUNITY_TELEMETRY_URL}{COMMUNITY_TELEMETRY_ENDPOINT}" + _, kwargs = mock_client.post.call_args + assert "api_key_fingerprint" in kwargs["json"] + assert kwargs["json"]["api_key_fingerprint"] is None + mock_client.post.assert_awaited_once_with( + url, + json={ + "tool_name": "tool", + "tool_args": {"a": 1}, + "client_name": "client", + "client_version": "1.0", + "protocol_version": "1.1", + "auth_origin": "none", + "api_key_fingerprint": None, + }, + headers=ANY, + timeout=2.0, + ) + + +@pytest.mark.asyncio +async def test_send_community_usage_report_raw_key_never_in_payload(monkeypatch): + """A representative raw key value never appears anywhere in the posted payload.""" + monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) + monkeypatch.setattr(config, "mixpanel_token", "", raising=False) + raw_key = "super-secret-raw-pro-api-key-value" + fingerprint = "b" * 64 + mock_client = AsyncMock() + mock_ctx_mgr = AsyncMock() + mock_ctx_mgr.__aenter__.return_value = mock_client + with patch("httpx.AsyncClient", return_value=mock_ctx_mgr): + await telemetry.send_community_usage_report( + "tool", + {"a": 1}, + "client", + "1.0", + "1.1", + auth_origin="client", + api_key_fingerprint=fingerprint, + ) + _, kwargs = mock_client.post.call_args + posted_payload = kwargs["json"] + assert raw_key not in str(posted_payload) + assert posted_payload["api_key_fingerprint"] == fingerprint + + # --------------------------------------------------------------------------- # send_community_resource_report tests # --------------------------------------------------------------------------- @@ -95,6 +197,44 @@ async def test_send_community_resource_report_sent(monkeypatch): "client_name": "client", "client_version": "1.0", "protocol_version": "1.1", + "auth_origin": None, + "api_key_fingerprint": None, + }, + headers=ANY, + timeout=2.0, + ) + + +@pytest.mark.asyncio +async def test_send_community_resource_report_forwards_auth_origin_and_fingerprint(monkeypatch): + """auth_origin and api_key_fingerprint passed in are forwarded verbatim to the delegated call.""" + monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) + monkeypatch.setattr(config, "mixpanel_token", "", raising=False) + uri = "blockscout-mcp://skill/SKILL.md" + fingerprint = "c" * 64 + mock_client = AsyncMock() + mock_ctx_mgr = AsyncMock() + mock_ctx_mgr.__aenter__.return_value = mock_client + with patch("httpx.AsyncClient", return_value=mock_ctx_mgr): + await telemetry.send_community_resource_report( + uri, + "client", + "1.0", + "1.1", + auth_origin="server", + api_key_fingerprint=fingerprint, + ) + url = f"{COMMUNITY_TELEMETRY_URL}{COMMUNITY_TELEMETRY_ENDPOINT}" + mock_client.post.assert_awaited_once_with( + url, + json={ + "tool_name": RESOURCE_READ_EVENT, + "tool_args": {"uri": uri}, + "client_name": "client", + "client_version": "1.0", + "protocol_version": "1.1", + "auth_origin": "server", + "api_key_fingerprint": fingerprint, }, headers=ANY, timeout=2.0, From 26d0adeff2c29c13d65f6fb74d0dbb45063eafb2 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 16:11:19 -0600 Subject: [PATCH 05/26] Phase 5: Observability wiring (decorators and resource reads) Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/observability.py | 5 ++++ blockscout_mcp_server/tools/decorators.py | 5 ++++ tests/test_observability.py | 26 ++++++++++++++++++ tests/tools/test_decorators.py | 33 ++++++++++++++++++++++- 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/blockscout_mcp_server/observability.py b/blockscout_mcp_server/observability.py index aab8c931..bf986cc8 100644 --- a/blockscout_mcp_server/observability.py +++ b/blockscout_mcp_server/observability.py @@ -23,6 +23,7 @@ extract_client_meta_from_ctx, format_client_meta_suffix, ) +from blockscout_mcp_server.pro_api_key_context import compute_api_key_fingerprint, compute_auth_origin from blockscout_mcp_server.resources import skill_resources logger = logging.getLogger(__name__) @@ -89,12 +90,16 @@ def log_resource_read(uri: Any, ctx: Any) -> None: # to store the task or switch to get_running_loop() on this path alone (RUF006): # that would re-introduce exactly the tool-vs-resource drift this feature prevents. try: + auth_origin = compute_auth_origin(ctx) + api_key_fingerprint = compute_api_key_fingerprint(ctx) asyncio.create_task( telemetry.send_community_resource_report( full_uri, meta.name, meta.version, meta.protocol, + auth_origin=auth_origin, + api_key_fingerprint=api_key_fingerprint, ) ) except Exception: diff --git a/blockscout_mcp_server/tools/decorators.py b/blockscout_mcp_server/tools/decorators.py index 9e262354..53d0d5c6 100644 --- a/blockscout_mcp_server/tools/decorators.py +++ b/blockscout_mcp_server/tools/decorators.py @@ -8,6 +8,7 @@ from blockscout_mcp_server import analytics, telemetry from blockscout_mcp_server.client_meta import extract_client_meta_from_ctx, format_client_meta_suffix +from blockscout_mcp_server.pro_api_key_context import compute_api_key_fingerprint, compute_auth_origin logger = logging.getLogger(__name__) @@ -49,6 +50,8 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: finally: try: arg_snapshot = arg_dict.copy() + auth_origin = compute_auth_origin(ctx) + api_key_fingerprint = compute_api_key_fingerprint(ctx) asyncio.create_task( telemetry.send_community_usage_report( func.__name__, @@ -56,6 +59,8 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: client_name, client_version, protocol_version, + auth_origin=auth_origin, + api_key_fingerprint=api_key_fingerprint, ) ) except Exception: diff --git a/tests/test_observability.py b/tests/test_observability.py index da12ff2a..e5686e36 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -120,6 +120,32 @@ def test_fan_out_both_sinks_called(): mock_create_task.assert_called_once() +def test_community_sink_forwards_auth_origin_and_fingerprint(monkeypatch): + """send_community_resource_report receives auth_origin/api_key_fingerprint derived from ctx.""" + monkeypatch.setattr("blockscout_mcp_server.pro_api_key_context.config.pro_api_key", "server-key", raising=False) + ctx = _make_ctx() + + # new_callable=MagicMock: send_community_resource_report is an async def, so a bare + # patch() would auto-create an AsyncMock whose call yields an (orphaned) coroutine + # when create_task is also mocked away. See the module-level idiom note above. + with ( + patch("blockscout_mcp_server.observability.analytics.track_resource_read"), + patch( + "blockscout_mcp_server.observability.telemetry.send_community_resource_report", + new_callable=MagicMock, + ) as mock_send_report, + patch("blockscout_mcp_server.observability.asyncio.create_task") as mock_create_task, + ): + log_resource_read(_SKILL_URI, ctx) + + mock_create_task.assert_called_once() + # The coroutine handed to create_task is the one produced by calling the mocked sink. + mock_send_report.assert_called_once() + call_kwargs = mock_send_report.call_args.kwargs + assert call_kwargs["auth_origin"] == "server" + assert call_kwargs["api_key_fingerprint"] is not None + + # --------------------------------------------------------------------------- # Error swallowing — no exception propagates # --------------------------------------------------------------------------- diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index 26848f2f..4f609585 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -125,7 +125,9 @@ async def dummy_tool(a: int, ctx: Context) -> int: "blockscout_mcp_server.tools.decorators.telemetry.send_community_usage_report", new_callable=AsyncMock, ) -async def test_decorator_reports_telemetry(mock_report, mock_ctx: Context) -> None: +async def test_decorator_reports_telemetry(mock_report, monkeypatch, mock_ctx: Context) -> None: + monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) + @log_tool_invocation async def dummy_tool(a: int, ctx: Context) -> int: return a @@ -140,9 +142,38 @@ async def dummy_tool(a: int, ctx: Context) -> int: UNDEFINED_CLIENT_NAME, UNDEFINED_CLIENT_VERSION, UNKNOWN_PROTOCOL_VERSION, + auth_origin="none", + api_key_fingerprint=None, ) +@pytest.mark.asyncio +@patch( + "blockscout_mcp_server.tools.decorators.telemetry.send_community_usage_report", + new_callable=AsyncMock, +) +async def test_decorator_reports_telemetry_with_client_key(mock_report, monkeypatch, mock_ctx: Context) -> None: + """A client-supplied PRO API key header is forwarded as a non-reversible fingerprint.""" + monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) + raw_key = "super-secret-client-key" + + @log_tool_invocation + async def dummy_tool(a: int, ctx: Context) -> int: + return a + + headers = Headers(headers={server_config.pro_api_key_header.upper(): raw_key}) + mock_ctx.session = None + mock_ctx.request_context = SimpleNamespace(request=SimpleNamespace(headers=headers)) + await dummy_tool(5, ctx=mock_ctx) + await asyncio.sleep(0) + + mock_report.assert_awaited_once() + call_kwargs = mock_report.await_args.kwargs + assert call_kwargs["auth_origin"] == "client" + assert call_kwargs["api_key_fingerprint"] is not None + assert call_kwargs["api_key_fingerprint"] != raw_key + + @pytest.mark.asyncio async def test_log_tool_invocation_logs_meta_fields(caplog: pytest.LogCaptureFixture, mock_ctx: Context) -> None: """Verify that MCP meta fields including OpenAI fields are logged in Tool invoked message.""" From 28c8b06f1b335dd0a05898da1e510963f68ca677 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 16:14:45 -0600 Subject: [PATCH 06/26] Phase 6: REST report-endpoint tests Co-Authored-By: Claude Opus 4.8 --- tests/api/test_routes.py | 116 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/tests/api/test_routes.py b/tests/api/test_routes.py index 0484bd99..69fe157c 100644 --- a/tests/api/test_routes.py +++ b/tests/api/test_routes.py @@ -734,6 +734,8 @@ async def test_report_tool_usage_success(mock_track, client: AsyncClient): "client_name": "cli", "client_version": "1.0", "protocol_version": "1.1", + "auth_origin": "client", + "api_key_fingerprint": "a" * 64, } headers = {"User-Agent": "BlockscoutMCP/0.0"} response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) @@ -745,6 +747,8 @@ async def test_report_tool_usage_success(mock_track, client: AsyncClient): assert kwargs["report"].client_name == "cli" assert kwargs["report"].client_version == "1.0" assert kwargs["report"].protocol_version == "1.1" + assert kwargs["report"].auth_origin == "client" + assert kwargs["report"].api_key_fingerprint == "a" * 64 assert kwargs["user_agent"] == headers["User-Agent"] @@ -767,6 +771,118 @@ async def test_report_tool_usage_missing_header(client: AsyncClient): assert response.status_code == 400 +@pytest.mark.asyncio +@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") +async def test_report_tool_usage_legacy_payload_without_new_fields(mock_track, client: AsyncClient): + """A legacy payload that omits auth_origin/api_key_fingerprint is still accepted. + + The analytics layer renders a None auth_origin as 'unknown', so the forwarded report + must carry None rather than being rejected outright. + """ + payload = { + "tool_name": "dummy", + "tool_args": {"a": 1}, + "client_name": "cli", + "client_version": "1.0", + "protocol_version": "1.1", + } + headers = {"User-Agent": "BlockscoutMCP/0.0"} + response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + assert response.status_code == 202 + mock_track.assert_called_once() + _, kwargs = mock_track.call_args + assert kwargs["report"].auth_origin is None + + +@pytest.mark.asyncio +async def test_report_tool_usage_rejects_bad_auth_origin_enum(client: AsyncClient): + """An out-of-enum auth_origin value is rejected with a 422.""" + payload = { + "tool_name": "dummy", + "tool_args": {"a": 1}, + "client_name": "cli", + "client_version": "1.0", + "protocol_version": "1.1", + "auth_origin": "bogus", + } + headers = {"User-Agent": "BlockscoutMCP/0.0"} + response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + assert response.status_code == 422 + + +@pytest.mark.asyncio +async def test_report_tool_usage_rejects_malformed_fingerprint(client: AsyncClient): + """A syntactically invalid api_key_fingerprint is rejected with a 422.""" + payload = { + "tool_name": "dummy", + "tool_args": {"a": 1}, + "client_name": "cli", + "client_version": "1.0", + "protocol_version": "1.1", + "api_key_fingerprint": "not-a-hash", + } + headers = {"User-Agent": "BlockscoutMCP/0.0"} + response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + assert response.status_code == 422 + + +@pytest.mark.asyncio +@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") +async def test_report_tool_usage_explicit_none_auth_origin_string_with_null_fingerprint( + mock_track, client: AsyncClient +): + """The exact no-key wire shape an updated reporter sends: auth_origin="none" with a null + fingerprint, sent explicitly rather than omitted. + + This guards against the endpoint rejecting real no-key reports from updated reporters. + """ + payload = { + "tool_name": "dummy", + "tool_args": {"a": 1}, + "client_name": "cli", + "client_version": "1.0", + "protocol_version": "1.1", + "auth_origin": "none", + "api_key_fingerprint": None, + } + headers = {"User-Agent": "BlockscoutMCP/0.0"} + response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + assert response.status_code == 202 + mock_track.assert_called_once() + _, kwargs = mock_track.call_args + assert kwargs["report"].auth_origin == "none" + assert kwargs["report"].api_key_fingerprint is None + + +@pytest.mark.asyncio +@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") +async def test_report_tool_usage_explicit_null_auth_origin_with_null_fingerprint(mock_track, client: AsyncClient): + """A JSON null auth_origin (not the string "none") with a null fingerprint is accepted. + + Phase 4 adds both keys to the outbound payload unconditionally, so any caller that omits + the auth_origin keyword causes the sender to serialize a literal `"auth_origin": null` on + the wire. This is the HTTP-boundary counterpart of the Phase 1 explicit-auth_origin=None + model test, and it is the only route case that catches a `Literal[...]`-without-`| None` + typing regression, which would otherwise 422 a real report. + """ + payload = { + "tool_name": "dummy", + "tool_args": {"a": 1}, + "client_name": "cli", + "client_version": "1.0", + "protocol_version": "1.1", + "auth_origin": None, + "api_key_fingerprint": None, + } + headers = {"User-Agent": "BlockscoutMCP/0.0"} + response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + assert response.status_code == 202 + mock_track.assert_called_once() + _, kwargs = mock_track.call_args + assert kwargs["report"].auth_origin is None + assert kwargs["report"].api_key_fingerprint is None + + @pytest.mark.asyncio @patch("blockscout_mcp_server.api.routes.direct_api_call", new_callable=AsyncMock) async def test_direct_api_call_post_success(mock_tool, client: AsyncClient): From 616b7828914cacc10ad09a4157f3f7b560517453 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 16:20:10 -0600 Subject: [PATCH 07/26] Phase 7: Documentation Updates Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 3 ++- API.md | 2 ++ README.md | 1 + SPEC.md | 2 ++ 4 files changed, 7 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index a733625d..5ee35c4d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,7 +24,7 @@ mcp-server/ │ ├── telemetry.py # Fire-and-forget community telemetry reporting │ ├── client_meta.py # Shared client metadata extraction helpers and defaults │ ├── observability.py # Resource-read observability helpers -│ ├── pro_api_key_context.py # Request-scoped client-supplied PRO API key state, resolver, and @pro_api_key_scope decorator; per-invocation credit sink and @pro_api_credit_scope decorator +│ ├── pro_api_key_context.py # Request-scoped client-supplied PRO API key state, resolver, and @pro_api_key_scope decorator; per-invocation credit sink and @pro_api_credit_scope decorator; auth-origin and key-fingerprint helpers for analytics/telemetry │ ├── cache.py # Simple in-memory cache for chain data │ ├── web3_pool.py # Async Web3 connection pool manager │ ├── models.py # Defines standardized Pydantic models for all tool responses @@ -398,6 +398,7 @@ mcp-server/ * **`pro_api_key_context.py`**: * Owns request-scoped resolution of a client-supplied Blockscout PRO API key, kept separate from logging/observability. * Provides a `ContextVar` of the per-request client-key state, a normalization/validation helper, `extract_client_pro_api_key_from_ctx()`, `resolve_pro_api_key()` (precedence: valid client key → server key → not-configured error; malformed client key → terminal error, no fallback), and the `@pro_api_key_scope` decorator. + * Also provides `compute_auth_origin()` and `compute_api_key_fingerprint()`, the `ctx`-derived helpers consumed by the analytics and community-telemetry paths (which run outside `@pro_api_key_scope`). * Honored for any HTTP request that carries the configured header (MCP-over-HTTP or REST); the key is never logged or placed in cache keys. * Also defines the per-invocation credit-tracking symbols: `CreditSink`, the `_credit_sink` `ContextVar`, and the `@pro_api_credit_scope` decorator (a sibling of `@pro_api_key_scope`). * **`cache.py`**: diff --git a/API.md b/API.md index 48158deb..ab026451 100644 --- a/API.md +++ b/API.md @@ -671,6 +671,8 @@ Receive an anonymous tool usage report from a community-run server. | `client_name` | `string` | Yes | Name of the MCP client invoking the tool. | | `client_version` | `string` | Yes | Version of the MCP client. | | `protocol_version` | `string` | Yes | Model Context Protocol version used. | + | `auth_origin` | `string \| null` | No | Resolved authorization origin on the reporting instance: `client`, `server`, or `none`. Explicit `null` (or omitted, as from legacy reporters) is treated as `unknown` downstream. | + | `api_key_fingerprint` | `string \| null` | No | One-way, non-reversible SHA-256 fingerprint of the effective PRO API key available to back the request on the reporting instance — the request's authorization context, not whether the invoked tool consumed the key. Non-null values are exactly 64 lowercase hexadecimal characters; never the key itself; explicit `null` (or omitted) when no usable key was available. | - **Example Request** diff --git a/README.md b/README.md index 03f1365b..71cf3725 100644 --- a/README.md +++ b/README.md @@ -454,6 +454,7 @@ To help us improve the Blockscout MCP Server, community-run instances of the ser - The name of the tool being called (e.g., `get_block_number`). - The parameters provided to the tool. - The version of the Blockscout MCP Server being used. +- A one-way, non-reversible hash (SHA-256) of the PRO API key available to authorize the request, when one is present. This is a derived fingerprint only — the key itself is never transmitted and cannot be recovered from the hash. **What we DO NOT collect:** diff --git a/SPEC.md b/SPEC.md index b16aca0a..a6c07159 100644 --- a/SPEC.md +++ b/SPEC.md @@ -897,6 +897,7 @@ To gain insight into tool usage patterns, the server can optionally report tool - MCP protocol version. - Tool arguments (currently sent as-is, without truncation). - Call source: whether the tool was invoked by MCP or via the REST API. + - Authorization origin: whether the request was backed by a client-supplied PRO API key (`client`), the server-configured key (`server`), or no usable key (`none`). This dimension is orthogonal to the call source — the two compose to describe both how a call arrived and how it was authorized. Community-forwarded events that predate this signal are recorded as `unknown`. ##### Intent Inference from Tool Sequences @@ -935,6 +936,7 @@ Intent analytics should favor derived signals over raw text. - **HTTP mode**: Active only when both `BLOCKSCOUT_MIXPANEL_TOKEN` is not configured AND `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` is not set to true - **Mechanism**: To understand usage in the open-source community, these instances send an anonymous, "fire-and-forget" report to a central endpoint (`POST /v1/report_tool_usage`) on the official Blockscout MCP server. This report contains the tool name, tool arguments, the MCP client name and version, the model context protocol version, and the server's version. - **Central Processing**: The central server receives this report, uses the sender's IP address for geolocation, and forwards the event to Mixpanel with the client metadata, protocol version, and a `source` property of `"community"`. This allows us to gather valuable aggregate statistics without requiring every user to have a Mixpanel account. +- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. In this change the fingerprint is accepted by the central endpoint purely as a forward-compatible wire signal: it is neither forwarded to the third-party analytics service nor persisted. Consuming and retaining it for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (The exact field names, enum values, and hex-shape constraints are documented in `API.md`.) - **Opt-Out**: This community reporting can be completely disabled by setting the `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` environment variable to `true`. ##### Resource-Read Observability From 06d9519a7f643bd4d196e9c13b571b6e5015083b Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 16:22:02 -0600 Subject: [PATCH 08/26] Phase 8: Version bump Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/__init__.py | 2 +- pyproject.toml | 2 +- server.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/blockscout_mcp_server/__init__.py b/blockscout_mcp_server/__init__.py index 0a765d1c..6e671f85 100644 --- a/blockscout_mcp_server/__init__.py +++ b/blockscout_mcp_server/__init__.py @@ -1,4 +1,4 @@ # SPDX-License-Identifier: LicenseRef-Blockscout """Blockscout MCP Server package.""" -__version__ = "0.17.0.dev1" +__version__ = "0.17.0.dev2" diff --git a/pyproject.toml b/pyproject.toml index bcdb50c7..ba93c7de 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "blockscout-mcp-server" -version = "0.17.0.dev1" +version = "0.17.0.dev2" description = "MCP server for Blockscout" requires-python = ">=3.11" dependencies = [ diff --git a/server.json b/server.json index d58dac80..a6f87b9f 100644 --- a/server.json +++ b/server.json @@ -2,7 +2,7 @@ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-12-11/server.schema.json", "name": "com.blockscout/mcp-server", "description": "MCP server for Blockscout", - "version": "0.17.0.dev1", + "version": "0.17.0.dev2", "websiteUrl": "https://blockscout.com", "repository": { "url": "https://github.com/blockscout/mcp-server", From 1184a142bc365574ca509b41964cf0a501fa6e68 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 17:09:34 -0600 Subject: [PATCH 09/26] Refactor: unify auth-origin and fingerprint into compute_auth_signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both compute_auth_origin and compute_api_key_fingerprint independently extracted the ctx key state and re-implemented the same client→server precedence ladder, so the two signals could silently diverge if the precedence ever changed in one but not the other. Callers that need both (the tool decorator and the resource-read sink) also extracted the state twice per call. Introduce compute_auth_signals(ctx) -> tuple[AuthOrigin, str | None] as the single source of truth: one extraction pass, with the (origin, fingerprint) pair returned co-located per branch so they cannot disagree. compute_auth_origin / compute_api_key_fingerprint become thin views over it; the decorator and observability paths now unpack the pair from one call. The Mixpanel path keeps using compute_auth_origin (fingerprint stays out of the property bag). Add tests pinning the (origin, fingerprint) pair per precedence branch and asserting the thin wrappers stay in lockstep with the combined result. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/observability.py | 5 +- blockscout_mcp_server/pro_api_key_context.py | 74 ++++++++++--------- blockscout_mcp_server/tools/decorators.py | 5 +- .../test_pro_api_key_context_auth_signals.py | 54 ++++++++++++++ 4 files changed, 98 insertions(+), 40 deletions(-) diff --git a/blockscout_mcp_server/observability.py b/blockscout_mcp_server/observability.py index bf986cc8..6d835ec1 100644 --- a/blockscout_mcp_server/observability.py +++ b/blockscout_mcp_server/observability.py @@ -23,7 +23,7 @@ extract_client_meta_from_ctx, format_client_meta_suffix, ) -from blockscout_mcp_server.pro_api_key_context import compute_api_key_fingerprint, compute_auth_origin +from blockscout_mcp_server.pro_api_key_context import compute_auth_signals from blockscout_mcp_server.resources import skill_resources logger = logging.getLogger(__name__) @@ -90,8 +90,7 @@ def log_resource_read(uri: Any, ctx: Any) -> None: # to store the task or switch to get_running_loop() on this path alone (RUF006): # that would re-introduce exactly the tool-vs-resource drift this feature prevents. try: - auth_origin = compute_auth_origin(ctx) - api_key_fingerprint = compute_api_key_fingerprint(ctx) + auth_origin, api_key_fingerprint = compute_auth_signals(ctx) asyncio.create_task( telemetry.send_community_resource_report( full_uri, diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index ca92897f..c07db4b5 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -251,62 +251,68 @@ def _fingerprint_pro_api_key(key: str) -> str: ``hashlib.sha256`` takes a bytes-like object, not a ``str``; the prefix+encoding scheme is centralized here so both the client-key and - server-key branches of :func:`compute_api_key_fingerprint` share one - construction. The raw key is never returned, logged, or placed anywhere - but this hash input. + server-key branches of :func:`compute_auth_signals` share one construction. + The raw key is never returned, logged, or placed anywhere but this hash + input. """ return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 -def compute_auth_origin(ctx: Any) -> AuthOrigin: - """Derive the authorization origin for *ctx* directly from request headers. +def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: + """Derive the ``(auth_origin, api_key_fingerprint)`` pair for *ctx* in one pass. + + Single source of truth for both signals: each precedence branch returns the + origin and the matching fingerprint together, so the two can never disagree + (``"client"`` ↔ client hash, ``"server"`` ↔ server hash, ``"none"`` ↔ + ``None``). :func:`compute_auth_origin` and :func:`compute_api_key_fingerprint` + are thin views over this function; callers needing both should call this + directly to avoid extracting the key state twice. Never raises (delegates to :func:`extract_client_pro_api_key_from_ctx`, - which never raises) and never calls :func:`resolve_pro_api_key`. + which never raises) and never calls :func:`resolve_pro_api_key` (it reads the + request-scoped ContextVar — unset on this path — and raises on a malformed + key). The precedence mirrors :func:`resolve_pro_api_key`: - - Valid client key → ``"client"``. - - Malformed client key → ``"none"`` (no fallback to the server key for a - malformed submission — the request will fail at the PRO API chokepoint). - - No client key (absent) → ``"server"`` when ``config.pro_api_key`` is - truthy, otherwise ``"none"``. + - Valid client key → ``("client", )``. + - Malformed client key → ``("none", None)`` (no fallback to the server key + for a malformed submission — the request will fail at the PRO API + chokepoint anyway). + - No client key (absent) → ``("server", )`` when + ``config.pro_api_key`` is truthy, otherwise ``("none", None)``. + + The raw key is never returned, logged, or used anywhere but as the hash + input. """ state = extract_client_pro_api_key_from_ctx(ctx) if isinstance(state, _Valid): - return "client" + return "client", _fingerprint_pro_api_key(state.value) if isinstance(state, _Malformed): - return "none" + return "none", None # _Absent — fall back to whether a server key is configured. - return "server" if config.pro_api_key else "none" - - -def compute_api_key_fingerprint(ctx: Any) -> str | None: - """Derive a one-way fingerprint of the effective PRO API key for *ctx*. + if config.pro_api_key: + return "server", _fingerprint_pro_api_key(config.pro_api_key) + return "none", None - Never raises and never calls :func:`resolve_pro_api_key`. Mirrors the - precedence in :func:`compute_auth_origin`: - - Valid client key → hash of the client value. - - Malformed client key → ``None`` (consistent with ``auth_origin = "none"``). - - Absent → hash of ``config.pro_api_key`` when truthy, otherwise ``None``. +def compute_auth_origin(ctx: Any) -> AuthOrigin: + """Return only the authorization origin for *ctx*. - The raw key is never returned, logged, or used anywhere but as the hash - input. + Thin view over :func:`compute_auth_signals` for call sites that consume the + origin alone (e.g. the Mixpanel property bag, where the fingerprint is + deliberately not emitted). """ - state = extract_client_pro_api_key_from_ctx(ctx) + return compute_auth_signals(ctx)[0] - if isinstance(state, _Valid): - return _fingerprint_pro_api_key(state.value) - if isinstance(state, _Malformed): - return None +def compute_api_key_fingerprint(ctx: Any) -> str | None: + """Return only the one-way fingerprint of the effective PRO API key for *ctx*. - # _Absent — fall back to the configured server key, if any. - if config.pro_api_key: - return _fingerprint_pro_api_key(config.pro_api_key) - return None + Thin view over :func:`compute_auth_signals`. + """ + return compute_auth_signals(ctx)[1] # --------------------------------------------------------------------------- diff --git a/blockscout_mcp_server/tools/decorators.py b/blockscout_mcp_server/tools/decorators.py index 53d0d5c6..256d0aec 100644 --- a/blockscout_mcp_server/tools/decorators.py +++ b/blockscout_mcp_server/tools/decorators.py @@ -8,7 +8,7 @@ from blockscout_mcp_server import analytics, telemetry from blockscout_mcp_server.client_meta import extract_client_meta_from_ctx, format_client_meta_suffix -from blockscout_mcp_server.pro_api_key_context import compute_api_key_fingerprint, compute_auth_origin +from blockscout_mcp_server.pro_api_key_context import compute_auth_signals logger = logging.getLogger(__name__) @@ -50,8 +50,7 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: finally: try: arg_snapshot = arg_dict.copy() - auth_origin = compute_auth_origin(ctx) - api_key_fingerprint = compute_api_key_fingerprint(ctx) + auth_origin, api_key_fingerprint = compute_auth_signals(ctx) asyncio.create_task( telemetry.send_community_usage_report( func.__name__, diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index d055afd7..ba98502e 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -18,6 +18,7 @@ from blockscout_mcp_server.pro_api_key_context import ( compute_api_key_fingerprint, compute_auth_origin, + compute_auth_signals, ) _HEADER_NAME = "Blockscout-MCP-Pro-Api-Key" @@ -210,3 +211,56 @@ def test_fingerprint_prefix_actually_participates(monkeypatch): fingerprint = compute_api_key_fingerprint(ctx) unprefixed_hash = hashlib.sha256(raw_key.encode("utf-8")).hexdigest() assert fingerprint != unprefixed_hash + + +# =========================================================================== +# compute_auth_signals — the single source of truth for both signals +# =========================================================================== +# +# compute_auth_origin / compute_api_key_fingerprint are thin views over this +# function. These cases pin the (origin, fingerprint) pairing per branch so the +# two signals can never silently diverge, and assert the two thin wrappers stay +# in lockstep with the combined result on every branch (a server key is +# configured throughout so a server-fallback regression cannot hide). + + +def test_auth_signals_valid_client_returns_client_and_client_hash(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_auth_signals(ctx) == ("client", _expected_fingerprint("client-key-123")) + + +def test_auth_signals_malformed_returns_none_and_no_fingerprint(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") + assert compute_auth_signals(ctx) == ("none", None) + + +def test_auth_signals_absent_with_server_key_returns_server_and_server_hash(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_auth_signals(ctx) == ("server", _expected_fingerprint("server-key")) + + +def test_auth_signals_absent_with_no_server_key_returns_none_and_no_fingerprint(monkeypatch): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_auth_signals(ctx) == ("none", None) + + +def test_thin_wrappers_match_combined_signals_on_every_branch(monkeypatch): + """compute_auth_origin / compute_api_key_fingerprint must equal the combined pair.""" + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + for ctx in ( + _ctx_with_header(_HEADER_NAME, "client-key-123"), # valid client + _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey"), # malformed + _ctx_with_header(_HEADER_NAME, ""), # absent -> server fallback + ): + origin, fingerprint = compute_auth_signals(ctx) + assert compute_auth_origin(ctx) == origin + assert compute_api_key_fingerprint(ctx) == fingerprint From 8ee8b2cb4e222c98fa3625ab86a69ecad345bbb3 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 17:34:03 -0600 Subject: [PATCH 10/26] perf(telemetry): derive auth signals once per invocation Address PR #424 review findings: - De-duplicate auth-origin/fingerprint derivation on the hot path. The tool decorator and the resource-read observability path now compute compute_auth_signals(ctx) once and thread the origin into track_tool_invocation / track_resource_read (which fall back to deriving it themselves for direct callers), reusing the fingerprint for the community report. Previously each tracked call extracted the headers twice and ran two SHA-256 hashes (one discarded inside compute_auth_origin). Mirrors the existing "compute client_meta once and thread it through" pattern. - Document compute_api_key_fingerprint as an intentionally-retained public view kept for the deferred fingerprint consumer (no production caller yet). - Clarify the README privacy section so "we do not collect secrets/keys" no longer reads as contradicting the new one-way key fingerprint. Adds a regression test asserting the signals are derived once and the same values reach both sinks. Version intentionally not re-bumped (follow-up commit within the already-versioned PR). Co-Authored-By: Claude Opus 4.8 --- README.md | 2 +- blockscout_mcp_server/analytics.py | 21 ++++++--- blockscout_mcp_server/observability.py | 15 ++++++- blockscout_mcp_server/pro_api_key_context.py | 7 ++- blockscout_mcp_server/tools/decorators.py | 10 ++++- tests/test_observability.py | 2 +- tests/tools/test_decorators.py | 47 +++++++++++++++++++- 7 files changed, 92 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 71cf3725..82a4950b 100644 --- a/README.md +++ b/README.md @@ -458,7 +458,7 @@ To help us improve the Blockscout MCP Server, community-run instances of the ser **What we DO NOT collect:** -- We do not collect any personal data, IP addresses (the central server uses the sender's IP for geolocation via Mixpanel and then discards it), secrets, or private keys. +- We do not collect any personal data, IP addresses (the central server uses the sender's IP for geolocation via Mixpanel and then discards it), or secrets and private keys themselves. The PRO API key in particular is never transmitted — only its one-way, non-reversible fingerprint described above, from which the key cannot be recovered. ### How to Opt-Out diff --git a/blockscout_mcp_server/analytics.py b/blockscout_mcp_server/analytics.py index 41d0b110..ac2b1f3a 100644 --- a/blockscout_mcp_server/analytics.py +++ b/blockscout_mcp_server/analytics.py @@ -37,7 +37,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: # noqa: D401 - simple pl get_header_case_insensitive, ) from blockscout_mcp_server.config import config -from blockscout_mcp_server.constants import AUTH_ORIGIN_UNKNOWN, RESOURCE_READ_EVENT +from blockscout_mcp_server.constants import AUTH_ORIGIN_UNKNOWN, RESOURCE_READ_EVENT, AuthOrigin from blockscout_mcp_server.models import ToolUsageReport from blockscout_mcp_server.pro_api_key_context import compute_auth_origin @@ -192,8 +192,16 @@ def track_tool_invocation( tool_name: str, tool_args: dict[str, Any], client_meta: ClientMeta | None = None, + auth_origin: AuthOrigin | None = None, ) -> None: - """Track a tool invocation in Mixpanel, if enabled and in HTTP mode.""" + """Track a tool invocation in Mixpanel, if enabled and in HTTP mode. + + ``auth_origin`` may be supplied pre-computed by the caller — the tool + decorator already derives the auth signals once per invocation (and reuses + them for the community report), so it threads the origin in here to avoid a + second ``ctx`` extraction and SHA-256 hash on the hot path. When omitted + (e.g. direct callers) it is derived from ``ctx`` here, preserving behavior. + """ if not _is_http_mode_enabled: return mp = _get_mixpanel_client() @@ -226,7 +234,7 @@ def track_tool_invocation( "tool_args": tool_args, "protocol_version": protocol_version, "source": _determine_call_source(ctx), - "auth_origin": compute_auth_origin(ctx), + "auth_origin": auth_origin if auth_origin is not None else compute_auth_origin(ctx), } meta = {"ip": ip} if ip else None @@ -243,15 +251,18 @@ def track_resource_read( ctx: Any, uri: str, client_meta: ClientMeta | None = None, + auth_origin: AuthOrigin | None = None, ) -> None: """Track a resource read in Mixpanel, if enabled and in HTTP mode. Delegates to :func:`track_tool_invocation` using the ``RESOURCE_READ`` event sentinel so that all gating logic (HTTP-mode, token, IP extraction, etc.) is reused verbatim. The caller is responsible for providing a fully-normalised - URI string — this function does not stringify. + URI string — this function does not stringify. ``auth_origin`` is threaded + through to :func:`track_tool_invocation` (see its docstring) so the resource + observability path also derives the auth signals only once per read. """ - track_tool_invocation(ctx, RESOURCE_READ_EVENT, {"uri": uri}, client_meta=client_meta) + track_tool_invocation(ctx, RESOURCE_READ_EVENT, {"uri": uri}, client_meta=client_meta, auth_origin=auth_origin) def track_community_usage(report: ToolUsageReport, ip: str, user_agent: str) -> None: diff --git a/blockscout_mcp_server/observability.py b/blockscout_mcp_server/observability.py index 6d835ec1..8e577ea9 100644 --- a/blockscout_mcp_server/observability.py +++ b/blockscout_mcp_server/observability.py @@ -70,9 +70,21 @@ def log_resource_read(uri: Any, ctx: Any) -> None: except Exception: pass + # Derive the auth-origin / fingerprint signals once and reuse them for both + # sinks below (mirrors @log_tool_invocation). The request headers are + # immutable for the call, so a single extraction (and single SHA-256) + # suffices. compute_auth_signals never raises, but guard it anyway so this + # observability path can never propagate into the request even if that + # contract changes; the (None, None) fallback degrades gracefully — the + # analytics sink re-derives the origin from ctx, the report omits the hash. + try: + auth_origin, api_key_fingerprint = compute_auth_signals(ctx) + except Exception: + auth_origin, api_key_fingerprint = None, None + # Step 2 — direct analytics sink (self-gating, synchronous). try: - analytics.track_resource_read(ctx, full_uri, client_meta=meta) + analytics.track_resource_read(ctx, full_uri, client_meta=meta, auth_origin=auth_origin) except Exception: pass @@ -90,7 +102,6 @@ def log_resource_read(uri: Any, ctx: Any) -> None: # to store the task or switch to get_running_loop() on this path alone (RUF006): # that would re-introduce exactly the tool-vs-resource drift this feature prevents. try: - auth_origin, api_key_fingerprint = compute_auth_signals(ctx) asyncio.create_task( telemetry.send_community_resource_report( full_uri, diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index c07db4b5..771581ad 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -310,7 +310,12 @@ def compute_auth_origin(ctx: Any) -> AuthOrigin: def compute_api_key_fingerprint(ctx: Any) -> str | None: """Return only the one-way fingerprint of the effective PRO API key for *ctx*. - Thin view over :func:`compute_auth_signals`. + Thin view over :func:`compute_auth_signals`. Intentionally retained for API + symmetry with :func:`compute_auth_origin`: today's sinks read the fingerprint + via :func:`compute_auth_signals` directly, so this wrapper has no production + caller yet — it is kept on purpose as the ready-made entry point for the + deferred fingerprint consumer (the follow-up that forwards/persists the + fingerprint for unique-user identity). Do not prune as unused. """ return compute_auth_signals(ctx)[1] diff --git a/blockscout_mcp_server/tools/decorators.py b/blockscout_mcp_server/tools/decorators.py index 256d0aec..19b02d93 100644 --- a/blockscout_mcp_server/tools/decorators.py +++ b/blockscout_mcp_server/tools/decorators.py @@ -30,6 +30,14 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: client_version = meta.version protocol_version = meta.protocol + # Derive the auth-origin / fingerprint signals once for this invocation + # and reuse them for both sinks below. The request headers are immutable + # for the lifetime of the call, so a single extraction (and single + # SHA-256) suffices — mirroring how `meta` above is computed once and + # threaded into both the analytics and community-telemetry paths. + # compute_auth_signals never raises (see its docstring). + auth_origin, api_key_fingerprint = compute_auth_signals(ctx) + # Track analytics (no-op if disabled) try: analytics.track_tool_invocation( @@ -37,6 +45,7 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: func.__name__, arg_dict, client_meta=meta, + auth_origin=auth_origin, ) except Exception: # Defensive: tracking must never break tool execution @@ -50,7 +59,6 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: finally: try: arg_snapshot = arg_dict.copy() - auth_origin, api_key_fingerprint = compute_auth_signals(ctx) asyncio.create_task( telemetry.send_community_usage_report( func.__name__, diff --git a/tests/test_observability.py b/tests/test_observability.py index e5686e36..f77dbc36 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -83,7 +83,7 @@ def test_uri_normalisation_anyurl_becomes_str(): ctx = _make_ctx() captured: list[str] = [] - def fake_track(ctx_, uri_, client_meta=None): + def fake_track(ctx_, uri_, client_meta=None, auth_origin=None): captured.append(uri_) with ( diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index 4f609585..8c93bee7 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -28,11 +28,12 @@ async def test_decorator_calls_analytics(monkeypatch, caplog: pytest.LogCaptureF calls = {} - def fake_track(ctx, name, args, client_meta=None): # type: ignore[no-untyped-def] + def fake_track(ctx, name, args, client_meta=None, auth_origin=None): # type: ignore[no-untyped-def] calls["ctx"] = ctx calls["name"] = name calls["args"] = args calls["client_meta"] = client_meta + calls["auth_origin"] = auth_origin monkeypatch.setattr("blockscout_mcp_server.tools.decorators.analytics.track_tool_invocation", fake_track) @@ -48,6 +49,9 @@ async def dummy_tool(a: int, ctx: Context) -> int: assert calls["args"] == {"a": 7} assert calls["ctx"] is mock_ctx assert "client_meta" in calls + # The decorator derives the auth signals once and threads the origin into + # track_tool_invocation (rather than letting it re-derive from ctx). + assert calls["auth_origin"] in ("client", "server", "none") @pytest.mark.asyncio @@ -174,6 +178,47 @@ async def dummy_tool(a: int, ctx: Context) -> int: assert call_kwargs["api_key_fingerprint"] != raw_key +@pytest.mark.asyncio +@patch( + "blockscout_mcp_server.tools.decorators.telemetry.send_community_usage_report", + new_callable=AsyncMock, +) +async def test_decorator_derives_auth_signals_once_and_threads_to_both_sinks( + mock_report, monkeypatch, mock_ctx: Context +) -> None: + """The (auth_origin, fingerprint) pair is derived a single time per invocation and the + identical values flow to both the Mixpanel sink and the community report — no second + ctx extraction / SHA-256 on the hot path (regression guard for the de-dup fix).""" + mock_signals = MagicMock(return_value=("client", "f" * 64)) + monkeypatch.setattr("blockscout_mcp_server.tools.decorators.compute_auth_signals", mock_signals) + + captured = {} + + def fake_track(ctx, name, args, client_meta=None, auth_origin=None): # type: ignore[no-untyped-def] + captured["auth_origin"] = auth_origin + + monkeypatch.setattr("blockscout_mcp_server.tools.decorators.analytics.track_tool_invocation", fake_track) + + @log_tool_invocation + async def dummy_tool(a: int, ctx: Context) -> int: + return a + + mock_ctx.session = None + mock_ctx.request_context = None + await dummy_tool(5, ctx=mock_ctx) + await asyncio.sleep(0) + + # Derived exactly once for the whole invocation. + mock_signals.assert_called_once() + # The same origin reached the analytics sink... + assert captured["auth_origin"] == "client" + # ...and the identical pair reached the community report. + mock_report.assert_awaited_once() + call_kwargs = mock_report.await_args.kwargs + assert call_kwargs["auth_origin"] == "client" + assert call_kwargs["api_key_fingerprint"] == "f" * 64 + + @pytest.mark.asyncio async def test_log_tool_invocation_logs_meta_fields(caplog: pytest.LogCaptureFixture, mock_ctx: Context) -> None: """Verify that MCP meta fields including OpenAI fields are logged in Tool invoked message.""" From bc751da6ce0fea6b6df9bc42cd12f5bff80ab8ac Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 18:04:26 -0600 Subject: [PATCH 11/26] Address review: harden auth-signal derivation, drop redundant validator, strengthen tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the auth-origin / api-key-fingerprint telemetry work, applying code-review findings: - decorators.py: guard compute_auth_signals(ctx) in try/except with a (None, None) fallback, mirroring the resource-read path in observability.py, so this observability concern can never propagate into the tool body if the "never raises" contract later changes. - models.py / constants.py: drop the redundant max_length on the api_key_fingerprint field (the anchored pattern ^[0-9a-f]{64}$ already fixes the length to exactly 64) and remove the now-unused PRO_API_KEY_FINGERPRINT_MAX_LENGTH constant. - models.py: align auth_origin / api_key_fingerprint field descriptions with API.md — "available to back the reported call", not "used for" — clarifying the signals reflect the request's authorization context, not key consumption. - tests: replace the vacuous auth_origin membership assertion with an exact "client" check; rewrite the "derive once" regression guard to drive the real analytics sink (only Mixpanel mocked) and spy on the real helpers so a dropped auth_origin thread is actually caught; pin the exact server-key fingerprint at the resource-read forwarding boundary instead of asserting merely non-null. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/constants.py | 6 -- blockscout_mcp_server/models.py | 14 ++-- blockscout_mcp_server/tools/decorators.py | 12 +++- tests/test_observability.py | 9 ++- tests/tools/test_decorators.py | 88 +++++++++++++++++------ 5 files changed, 92 insertions(+), 37 deletions(-) diff --git a/blockscout_mcp_server/constants.py b/blockscout_mcp_server/constants.py index 348c09bc..a50cac25 100644 --- a/blockscout_mcp_server/constants.py +++ b/blockscout_mcp_server/constants.py @@ -124,9 +124,3 @@ # reports that predate the `auth_origin` field. Deliberately not part of `AuthOrigin`: # it is never a valid report value and never returned by the helpers. AUTH_ORIGIN_UNKNOWN = "unknown" - -# A SHA-256 hex digest is always exactly 64 characters. This is the defensive -# max_length cap on the inbound community report's `api_key_fingerprint` field -# (mirroring the rationale documented for `_MAX_KEY_LENGTH` in pro_api_key_context.py); -# the field's `pattern` enforces the full exact shape. -PRO_API_KEY_FINGERPRINT_MAX_LENGTH = 64 diff --git a/blockscout_mcp_server/models.py b/blockscout_mcp_server/models.py index b067cd41..5059db9e 100644 --- a/blockscout_mcp_server/models.py +++ b/blockscout_mcp_server/models.py @@ -5,7 +5,7 @@ from pydantic import BaseModel, ConfigDict, Field -from blockscout_mcp_server.constants import PRO_API_KEY_FINGERPRINT_MAX_LENGTH, AuthOrigin +from blockscout_mcp_server.constants import AuthOrigin # --- Generic Type Variable --- T = TypeVar("T") @@ -20,19 +20,21 @@ class ToolUsageReport(BaseModel): auth_origin: AuthOrigin | None = Field( default=None, description=( - "The origin of the authorization used for the reported call: 'client' for a " + "The origin of the authorization available to back the reported call: 'client' for a " "client-supplied PRO API key, 'server' for a server-configured key, or 'none' for " - "no usable key. Absent on legacy payloads that predate this field." + "no usable key. Reflects the request's authorization context, not whether the invoked " + "tool actually consumed a key. Absent on legacy payloads that predate this field." ), ) api_key_fingerprint: str | None = Field( default=None, pattern=r"^[0-9a-f]{64}$", - max_length=PRO_API_KEY_FINGERPRINT_MAX_LENGTH, description=( "A one-way, non-reversible SHA-256 hex digest fingerprint of the effective PRO API " - "key used for the reported call, or null if no key was used. This field is accepted " - "over the wire but is not yet consumed (not forwarded to Mixpanel, not persisted)." + "key available to back the reported call, or null if no usable key was available. " + "The anchored pattern already constrains this to exactly 64 lowercase hex characters. " + "This field is accepted over the wire but is not yet consumed (not forwarded to " + "Mixpanel, not persisted)." ), ) diff --git a/blockscout_mcp_server/tools/decorators.py b/blockscout_mcp_server/tools/decorators.py index 19b02d93..07ac2ebd 100644 --- a/blockscout_mcp_server/tools/decorators.py +++ b/blockscout_mcp_server/tools/decorators.py @@ -35,8 +35,16 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: # for the lifetime of the call, so a single extraction (and single # SHA-256) suffices — mirroring how `meta` above is computed once and # threaded into both the analytics and community-telemetry paths. - # compute_auth_signals never raises (see its docstring). - auth_origin, api_key_fingerprint = compute_auth_signals(ctx) + # compute_auth_signals never raises today (see its docstring), but guard + # it anyway so this observability concern can never propagate into the + # tool body even if that contract later changes — mirroring the identical + # guard on the resource-read path (observability.log_resource_read). The + # (None, None) fallback degrades gracefully: the analytics sink re-derives + # the origin from ctx, and the community report simply omits the hash. + try: + auth_origin, api_key_fingerprint = compute_auth_signals(ctx) + except Exception: + auth_origin, api_key_fingerprint = None, None # Track analytics (no-op if disabled) try: diff --git a/tests/test_observability.py b/tests/test_observability.py index f77dbc36..860ee024 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -3,6 +3,7 @@ from __future__ import annotations +import hashlib import logging from unittest.mock import MagicMock, patch @@ -15,6 +16,7 @@ ClientMeta, format_client_meta_suffix, ) +from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX from blockscout_mcp_server.observability import log_resource_read _SKILL_URI = "blockscout-mcp://skill/SKILL.md" @@ -143,7 +145,12 @@ def test_community_sink_forwards_auth_origin_and_fingerprint(monkeypatch): mock_send_report.assert_called_once() call_kwargs = mock_send_report.call_args.kwargs assert call_kwargs["auth_origin"] == "server" - assert call_kwargs["api_key_fingerprint"] is not None + # Pin the exact server-key fingerprint at this forwarding boundary (not merely "not None"), + # so a regression that forwards a different non-None hash here — e.g. an unprefixed digest or + # the client-key hash — is caught. The value is otherwise only asserted in the helper's own + # unit tests, never at this sink's boundary. + expected_fingerprint = hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}server-key".encode()).hexdigest() + assert call_kwargs["api_key_fingerprint"] == expected_fingerprint # --------------------------------------------------------------------------- diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index 8c93bee7..ccf78c38 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -1,5 +1,6 @@ # SPDX-License-Identifier: LicenseRef-Blockscout import asyncio +import hashlib import logging from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -17,7 +18,13 @@ UNKNOWN_PROTOCOL_VERSION, ) from blockscout_mcp_server.config import config as server_config -from blockscout_mcp_server.pro_api_key_context import pro_api_key_scope, resolve_pro_api_key +from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX +from blockscout_mcp_server.pro_api_key_context import ( + compute_auth_origin, + compute_auth_signals, + pro_api_key_scope, + resolve_pro_api_key, +) from blockscout_mcp_server.tools.decorators import log_tool_invocation @@ -41,6 +48,15 @@ def fake_track(ctx, name, args, client_meta=None, auth_origin=None): # type: ig async def dummy_tool(a: int, ctx: Context) -> int: return a + # A valid client PRO API key header makes the derived origin deterministic ("client"). + # Pinning the exact value is what gives the assertion teeth: `in ("client","server","none")` + # is vacuous because every AuthOrigin member trivially satisfies it, so it would pass even + # if the decorator threaded a constant or the wrong field. + monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) + headers = Headers(headers={server_config.pro_api_key_header.upper(): "client-key-123"}) + mock_ctx.session = None + mock_ctx.request_context = SimpleNamespace(request=SimpleNamespace(headers=headers)) + # Act await dummy_tool(7, ctx=mock_ctx) @@ -49,9 +65,10 @@ async def dummy_tool(a: int, ctx: Context) -> int: assert calls["args"] == {"a": 7} assert calls["ctx"] is mock_ctx assert "client_meta" in calls - # The decorator derives the auth signals once and threads the origin into - # track_tool_invocation (rather than letting it re-derive from ctx). - assert calls["auth_origin"] in ("client", "server", "none") + # The decorator derives the auth signals from ctx and threads the origin into + # track_tool_invocation. With a valid client-key header present, that origin must be + # exactly "client" — proving a real ctx-derived value was threaded, not a constant. + assert calls["auth_origin"] == "client" @pytest.mark.asyncio @@ -187,36 +204,63 @@ async def test_decorator_derives_auth_signals_once_and_threads_to_both_sinks( mock_report, monkeypatch, mock_ctx: Context ) -> None: """The (auth_origin, fingerprint) pair is derived a single time per invocation and the - identical values flow to both the Mixpanel sink and the community report — no second - ctx extraction / SHA-256 on the hot path (regression guard for the de-dup fix).""" - mock_signals = MagicMock(return_value=("client", "f" * 64)) - monkeypatch.setattr("blockscout_mcp_server.tools.decorators.compute_auth_signals", mock_signals) - - captured = {} - - def fake_track(ctx, name, args, client_meta=None, auth_origin=None): # type: ignore[no-untyped-def] - captured["auth_origin"] = auth_origin - - monkeypatch.setattr("blockscout_mcp_server.tools.decorators.analytics.track_tool_invocation", fake_track) + identical values flow to both the Mixpanel sink and the community report — with no second + ctx extraction / SHA-256 on the hot path (regression guard for the de-dup fix). + + This drives the *real* analytics sink (only the Mixpanel client is mocked) and spies on the + real derivation helpers rather than mocking them away. That is what makes the guard + meaningful: if the decorator stopped threading ``auth_origin`` into ``track_tool_invocation``, + the analytics sink's ``compute_auth_origin(ctx)`` fallback would fire — and + ``origin_fallback_spy.assert_not_called()`` would catch it. Mocking ``compute_auth_signals`` + away (the previous approach) made the "derived once" claim structurally true by construction + instead of observing it. + """ + # Spy on the single derivation point (the decorator) while keeping the real implementation. + signals_spy = MagicMock(side_effect=compute_auth_signals) + monkeypatch.setattr("blockscout_mcp_server.tools.decorators.compute_auth_signals", signals_spy) + # Spy on the analytics fallback; it must NEVER run because the origin is threaded in. + origin_fallback_spy = MagicMock(side_effect=compute_auth_origin) + monkeypatch.setattr("blockscout_mcp_server.analytics.compute_auth_origin", origin_fallback_spy) + + # Drive the real analytics sink with a mocked Mixpanel client so the property bag (and thus + # the fallback branch) is genuinely exercised rather than structurally bypassed. + mock_mp = MagicMock() + monkeypatch.setattr("blockscout_mcp_server.analytics._is_http_mode_enabled", True, raising=False) + monkeypatch.setattr("blockscout_mcp_server.analytics._get_mixpanel_client", lambda: mock_mp) + + # Deterministic valid client-key header -> origin "client", fingerprint = client-key hash. + monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) + raw_key = "client-key-123" + headers = Headers(headers={server_config.pro_api_key_header.upper(): raw_key}) + req = SimpleNamespace(headers=headers, client=SimpleNamespace(host="127.0.0.1")) + mock_ctx.session = None + mock_ctx.request_context = SimpleNamespace(request=req) @log_tool_invocation async def dummy_tool(a: int, ctx: Context) -> int: return a - mock_ctx.session = None - mock_ctx.request_context = None await dummy_tool(5, ctx=mock_ctx) await asyncio.sleep(0) - # Derived exactly once for the whole invocation. - mock_signals.assert_called_once() - # The same origin reached the analytics sink... - assert captured["auth_origin"] == "client" + # Derived exactly once by the decorator... + signals_spy.assert_called_once() + # ...and the analytics sink reused the threaded origin instead of re-deriving it. + origin_fallback_spy.assert_not_called() + + # The same origin reached the Mixpanel property bag (3rd positional arg of mp.track). + mock_mp.track.assert_called_once() + props = mock_mp.track.call_args.args[2] + assert props["auth_origin"] == "client" + # The fingerprint is never emitted to Mixpanel (privacy invariant). + assert "api_key_fingerprint" not in props + # ...and the identical pair reached the community report. + expected_fingerprint = hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{raw_key}".encode()).hexdigest() mock_report.assert_awaited_once() call_kwargs = mock_report.await_args.kwargs assert call_kwargs["auth_origin"] == "client" - assert call_kwargs["api_key_fingerprint"] == "f" * 64 + assert call_kwargs["api_key_fingerprint"] == expected_fingerprint @pytest.mark.asyncio From d4a17d2d3a2063dea5426278b31eaaa129cfbbf0 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Tue, 30 Jun 2026 18:27:42 -0600 Subject: [PATCH 12/26] Consolidate auth-signal derivation into a single gated helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both observability paths (the tool decorator and the resource-read sink) duplicated the same guarded `compute_auth_signals(ctx)` call. Extract it into `telemetry.resolve_auth_signals`, the single derivation point shared by both, so the ctx extraction, the SHA-256, and the defensive guard are written once. The helper also short-circuits to (None, None) — without touching ctx — when no sink can consume the signals (not HTTP mode AND community telemetry disabled), avoiding a per-call SHA-256 of the configured server key. Behavior is unchanged: in that state both sinks already short-circuit on their own gates before reading these values. Tests: re-point the "derived once" regression guard at the new single derivation point; add unit coverage for the gate (short-circuit, both derive branches, never-raises). Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/observability.py | 17 +++---- blockscout_mcp_server/telemetry.py | 37 ++++++++++++++ blockscout_mcp_server/tools/decorators.py | 23 +++------ tests/test_telemetry.py | 60 ++++++++++++++++++++++- tests/tools/test_decorators.py | 11 +++-- 5 files changed, 115 insertions(+), 33 deletions(-) diff --git a/blockscout_mcp_server/observability.py b/blockscout_mcp_server/observability.py index 8e577ea9..03260abf 100644 --- a/blockscout_mcp_server/observability.py +++ b/blockscout_mcp_server/observability.py @@ -23,7 +23,6 @@ extract_client_meta_from_ctx, format_client_meta_suffix, ) -from blockscout_mcp_server.pro_api_key_context import compute_auth_signals from blockscout_mcp_server.resources import skill_resources logger = logging.getLogger(__name__) @@ -71,16 +70,12 @@ def log_resource_read(uri: Any, ctx: Any) -> None: pass # Derive the auth-origin / fingerprint signals once and reuse them for both - # sinks below (mirrors @log_tool_invocation). The request headers are - # immutable for the call, so a single extraction (and single SHA-256) - # suffices. compute_auth_signals never raises, but guard it anyway so this - # observability path can never propagate into the request even if that - # contract changes; the (None, None) fallback degrades gracefully — the - # analytics sink re-derives the origin from ctx, the report omits the hash. - try: - auth_origin, api_key_fingerprint = compute_auth_signals(ctx) - except Exception: - auth_origin, api_key_fingerprint = None, None + # sinks below (mirrors @log_tool_invocation). telemetry.resolve_auth_signals + # centralizes the single ctx extraction + SHA-256, the defensive guard, and the + # all-telemetry-disabled short-circuit shared verbatim with the tool path. The + # (None, None) fallback degrades gracefully — the analytics sink re-derives the + # origin from ctx, the report omits the hash. + auth_origin, api_key_fingerprint = telemetry.resolve_auth_signals(ctx) # Step 2 — direct analytics sink (self-gating, synchronous). try: diff --git a/blockscout_mcp_server/telemetry.py b/blockscout_mcp_server/telemetry.py index 2ca1aa7b..8768b22b 100644 --- a/blockscout_mcp_server/telemetry.py +++ b/blockscout_mcp_server/telemetry.py @@ -1,5 +1,6 @@ # SPDX-License-Identifier: LicenseRef-Blockscout import logging +from typing import Any import httpx @@ -10,11 +11,47 @@ COMMUNITY_TELEMETRY_URL, RESOURCE_READ_EVENT, SERVER_VERSION, + AuthOrigin, ) +from blockscout_mcp_server.pro_api_key_context import compute_auth_signals logger = logging.getLogger(__name__) +def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: + """Derive the ``(auth_origin, api_key_fingerprint)`` pair for the observability sinks. + + Single entry point shared by both observability paths — ``log_tool_invocation`` + (the tool decorator) and ``log_resource_read`` — so the one ``ctx`` extraction + and SHA-256 (see :func:`blockscout_mcp_server.pro_api_key_context.compute_auth_signals`), + the defensive guard, and the short-circuit below are written once instead of + duplicated at each site. + + Returns ``(None, None)`` *without touching* ``ctx`` when no sink can consume the + signals — analytics is off (not HTTP mode) **and** community telemetry is + disabled. In that state both sinks short-circuit on their own gates before + reading these values, so skipping derivation changes nothing that is emitted + while avoiding a per-call SHA-256 of the configured server key. The guard is + deliberately a *superset* of the precise per-sink gates (it may still derive in + a rare config where only the suppressed sink would have run); erring toward + deriving keeps this cheap check independent of the sinks' internal logic. When + HTTP mode is off the analytics sink early-returns before its ``ctx`` re-derivation + fallback, so a ``None`` origin from this short-circuit is never observed by it. + + Never raises: :func:`compute_auth_signals` is defensive today, but the guard is + kept so this observability concern can never propagate into the tool body even + if that contract later changes. The ``(None, None)`` fallback degrades gracefully + — the analytics sink re-derives the origin from ``ctx``, the community report + omits the hash. + """ + if not analytics.is_http_mode_enabled() and config.disable_community_telemetry: + return None, None + try: + return compute_auth_signals(ctx) + except Exception: + return None, None + + async def send_community_usage_report( tool_name: str, tool_args: dict, diff --git a/blockscout_mcp_server/tools/decorators.py b/blockscout_mcp_server/tools/decorators.py index 07ac2ebd..5762aa4e 100644 --- a/blockscout_mcp_server/tools/decorators.py +++ b/blockscout_mcp_server/tools/decorators.py @@ -8,7 +8,6 @@ from blockscout_mcp_server import analytics, telemetry from blockscout_mcp_server.client_meta import extract_client_meta_from_ctx, format_client_meta_suffix -from blockscout_mcp_server.pro_api_key_context import compute_auth_signals logger = logging.getLogger(__name__) @@ -30,21 +29,13 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: client_version = meta.version protocol_version = meta.protocol - # Derive the auth-origin / fingerprint signals once for this invocation - # and reuse them for both sinks below. The request headers are immutable - # for the lifetime of the call, so a single extraction (and single - # SHA-256) suffices — mirroring how `meta` above is computed once and - # threaded into both the analytics and community-telemetry paths. - # compute_auth_signals never raises today (see its docstring), but guard - # it anyway so this observability concern can never propagate into the - # tool body even if that contract later changes — mirroring the identical - # guard on the resource-read path (observability.log_resource_read). The - # (None, None) fallback degrades gracefully: the analytics sink re-derives - # the origin from ctx, and the community report simply omits the hash. - try: - auth_origin, api_key_fingerprint = compute_auth_signals(ctx) - except Exception: - auth_origin, api_key_fingerprint = None, None + # Derive the auth-origin / fingerprint signals once for this invocation and + # reuse them for both sinks below — mirroring how `meta` above is computed + # once and threaded into both the analytics and community-telemetry paths. + # telemetry.resolve_auth_signals centralizes the single ctx extraction + + # SHA-256, the defensive guard, and the all-telemetry-disabled short-circuit + # (shared verbatim with the resource-read path, observability.log_resource_read). + auth_origin, api_key_fingerprint = telemetry.resolve_auth_signals(ctx) # Track analytics (no-op if disabled) try: diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 44c4ad59..0c10af33 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -1,5 +1,5 @@ # SPDX-License-Identifier: LicenseRef-Blockscout -from unittest.mock import ANY, AsyncMock, patch +from unittest.mock import ANY, AsyncMock, MagicMock, patch import pytest @@ -11,6 +11,64 @@ RESOURCE_READ_EVENT, ) +# --------------------------------------------------------------------------- +# resolve_auth_signals — shared derivation entry point + all-disabled short-circuit +# --------------------------------------------------------------------------- + + +def test_resolve_auth_signals_short_circuits_when_all_telemetry_disabled(monkeypatch): + """With HTTP mode off AND community telemetry disabled, derivation is skipped entirely. + + No sink can consume the signals in that state, so the helper must return + (None, None) without extracting ctx or hashing a key. + """ + monkeypatch.setattr(config, "disable_community_telemetry", True, raising=False) + analytics.set_http_mode(False) + compute_spy = MagicMock() + monkeypatch.setattr("blockscout_mcp_server.telemetry.compute_auth_signals", compute_spy) + + assert telemetry.resolve_auth_signals(object()) == (None, None) + compute_spy.assert_not_called() + + +def test_resolve_auth_signals_derives_when_community_enabled(monkeypatch): + """Community telemetry enabled (HTTP mode off) still needs the signals -> derivation runs.""" + monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) + analytics.set_http_mode(False) + compute_spy = MagicMock(return_value=("server", "d" * 64)) + monkeypatch.setattr("blockscout_mcp_server.telemetry.compute_auth_signals", compute_spy) + + ctx = object() + assert telemetry.resolve_auth_signals(ctx) == ("server", "d" * 64) + compute_spy.assert_called_once_with(ctx) + + +def test_resolve_auth_signals_derives_in_http_mode_even_if_community_disabled(monkeypatch): + """HTTP mode on feeds the analytics sink, so derivation runs even when community is disabled.""" + monkeypatch.setattr(config, "disable_community_telemetry", True, raising=False) + analytics.set_http_mode(True) + try: + compute_spy = MagicMock(return_value=("client", "e" * 64)) + monkeypatch.setattr("blockscout_mcp_server.telemetry.compute_auth_signals", compute_spy) + + ctx = object() + assert telemetry.resolve_auth_signals(ctx) == ("client", "e" * 64) + compute_spy.assert_called_once_with(ctx) + finally: + analytics.set_http_mode(False) + + +def test_resolve_auth_signals_never_raises(monkeypatch): + """A hypothetical failure in compute_auth_signals degrades to (None, None), never propagates.""" + monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) + analytics.set_http_mode(False) + monkeypatch.setattr( + "blockscout_mcp_server.telemetry.compute_auth_signals", + MagicMock(side_effect=RuntimeError("boom")), + ) + + assert telemetry.resolve_auth_signals(object()) == (None, None) + @pytest.mark.asyncio async def test_send_community_usage_report_sent(monkeypatch): diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index ccf78c38..63af2d4e 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -21,10 +21,10 @@ from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX from blockscout_mcp_server.pro_api_key_context import ( compute_auth_origin, - compute_auth_signals, pro_api_key_scope, resolve_pro_api_key, ) +from blockscout_mcp_server.telemetry import resolve_auth_signals from blockscout_mcp_server.tools.decorators import log_tool_invocation @@ -211,13 +211,14 @@ async def test_decorator_derives_auth_signals_once_and_threads_to_both_sinks( real derivation helpers rather than mocking them away. That is what makes the guard meaningful: if the decorator stopped threading ``auth_origin`` into ``track_tool_invocation``, the analytics sink's ``compute_auth_origin(ctx)`` fallback would fire — and - ``origin_fallback_spy.assert_not_called()`` would catch it. Mocking ``compute_auth_signals`` + ``origin_fallback_spy.assert_not_called()`` would catch it. Mocking the derivation away (the previous approach) made the "derived once" claim structurally true by construction instead of observing it. """ - # Spy on the single derivation point (the decorator) while keeping the real implementation. - signals_spy = MagicMock(side_effect=compute_auth_signals) - monkeypatch.setattr("blockscout_mcp_server.tools.decorators.compute_auth_signals", signals_spy) + # Spy on the single derivation point — the shared telemetry.resolve_auth_signals helper the + # decorator delegates to — while keeping the real implementation. + signals_spy = MagicMock(side_effect=resolve_auth_signals) + monkeypatch.setattr("blockscout_mcp_server.telemetry.resolve_auth_signals", signals_spy) # Spy on the analytics fallback; it must NEVER run because the origin is threaded in. origin_fallback_spy = MagicMock(side_effect=compute_auth_origin) monkeypatch.setattr("blockscout_mcp_server.analytics.compute_auth_origin", origin_fallback_spy) From 09f34196530ac0fb69b4e6a22453596ef6cf6fa6 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 18:32:31 -0600 Subject: [PATCH 13/26] Address code review on auth-origin/fingerprint usage analytics Follow-up to PR #424 (auth_origin + api_key_fingerprint in usage analytics, closes #423), applying the accepted review decisions: - Tolerate a malformed api_key_fingerprint instead of rejecting the report: a before-validator coerces any non-64-lowercase-hex value to null (report still 202), preserving the "present => valid" invariant; auth_origin stays strictly validated (422). - Gate the server-key fingerprint on its only consumer: derive it (and hash the server key) only when community reporting is enabled, threaded via resolve_auth_signals(include_server_fingerprint=...). - Remove compute_api_key_fingerprint; compute_auth_signals is the single source of truth for the (auth_origin, fingerprint) pair. - compute_auth_origin passes include_server_fingerprint=False so the origin-only path no longer computes a server-key SHA-256 it discards. - Drop the redundant ^/$ anchors from _FINGERPRINT_PATTERN (matched via fullmatch, behavior-identical). - Trim SPEC/API/AGENTS to one home-of-record: why in SPEC, field shape in API, search hint in AGENTS; mechanics live in code + tests. - Update and extend unit tests to pin all of the above. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 2 +- SPEC.md | 2 +- blockscout_mcp_server/models.py | 35 ++++- blockscout_mcp_server/pro_api_key_context.py | 41 +++--- blockscout_mcp_server/telemetry.py | 8 +- tests/api/test_routes.py | 14 +- .../test_pro_api_key_context_auth_signals.py | 128 ++++++++++-------- tests/test_telemetry.py | 39 +++++- tests/test_tool_usage_report.py | 56 +++++--- 9 files changed, 215 insertions(+), 110 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5ee35c4d..ae582840 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -398,7 +398,7 @@ mcp-server/ * **`pro_api_key_context.py`**: * Owns request-scoped resolution of a client-supplied Blockscout PRO API key, kept separate from logging/observability. * Provides a `ContextVar` of the per-request client-key state, a normalization/validation helper, `extract_client_pro_api_key_from_ctx()`, `resolve_pro_api_key()` (precedence: valid client key → server key → not-configured error; malformed client key → terminal error, no fallback), and the `@pro_api_key_scope` decorator. - * Also provides `compute_auth_origin()` and `compute_api_key_fingerprint()`, the `ctx`-derived helpers consumed by the analytics and community-telemetry paths (which run outside `@pro_api_key_scope`). + * Also provides the `ctx`-derived helpers `compute_auth_signals()` (community-telemetry path) and `compute_auth_origin()` (analytics path), which run outside `@pro_api_key_scope`. * Honored for any HTTP request that carries the configured header (MCP-over-HTTP or REST); the key is never logged or placed in cache keys. * Also defines the per-invocation credit-tracking symbols: `CreditSink`, the `_credit_sink` `ContextVar`, and the `@pro_api_credit_scope` decorator (a sibling of `@pro_api_key_scope`). * **`cache.py`**: diff --git a/SPEC.md b/SPEC.md index a6c07159..30701692 100644 --- a/SPEC.md +++ b/SPEC.md @@ -936,7 +936,7 @@ Intent analytics should favor derived signals over raw text. - **HTTP mode**: Active only when both `BLOCKSCOUT_MIXPANEL_TOKEN` is not configured AND `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` is not set to true - **Mechanism**: To understand usage in the open-source community, these instances send an anonymous, "fire-and-forget" report to a central endpoint (`POST /v1/report_tool_usage`) on the official Blockscout MCP server. This report contains the tool name, tool arguments, the MCP client name and version, the model context protocol version, and the server's version. - **Central Processing**: The central server receives this report, uses the sender's IP address for geolocation, and forwards the event to Mixpanel with the client metadata, protocol version, and a `source` property of `"community"`. This allows us to gather valuable aggregate statistics without requiring every user to have a Mixpanel account. -- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. In this change the fingerprint is accepted by the central endpoint purely as a forward-compatible wire signal: it is neither forwarded to the third-party analytics service nor persisted. Consuming and retaining it for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (The exact field names, enum values, and hex-shape constraints are documented in `API.md`.) +- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because nothing consumes the fingerprint yet, an invalid one is tolerated rather than allowed to drop an otherwise-valid report — preserving a "present ⇒ valid" invariant the follow-up will rely on — whereas the already-consumed `auth_origin` is validated strictly. Consuming and retaining it for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (The exact field names, enum values, and hex-shape constraints are documented in `API.md`.) - **Opt-Out**: This community reporting can be completely disabled by setting the `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` environment variable to `true`. ##### Resource-Read Observability diff --git a/blockscout_mcp_server/models.py b/blockscout_mcp_server/models.py index 5059db9e..c4143fb2 100644 --- a/blockscout_mcp_server/models.py +++ b/blockscout_mcp_server/models.py @@ -1,12 +1,20 @@ # SPDX-License-Identifier: LicenseRef-Blockscout """Pydantic models for standardized tool responses.""" +import logging +import re from typing import Any, Generic, TypeVar -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, field_validator from blockscout_mcp_server.constants import AuthOrigin +logger = logging.getLogger(__name__) + +# A fingerprint is exactly 64 lowercase hex characters — the shape of a +# hashlib.sha256(...).hexdigest() digest. +_FINGERPRINT_PATTERN = re.compile(r"[0-9a-f]{64}") + # --- Generic Type Variable --- T = TypeVar("T") @@ -28,16 +36,33 @@ class ToolUsageReport(BaseModel): ) api_key_fingerprint: str | None = Field( default=None, - pattern=r"^[0-9a-f]{64}$", description=( "A one-way, non-reversible SHA-256 hex digest fingerprint of the effective PRO API " "key available to back the reported call, or null if no usable key was available. " - "The anchored pattern already constrains this to exactly 64 lowercase hex characters. " - "This field is accepted over the wire but is not yet consumed (not forwarded to " - "Mixpanel, not persisted)." + "A valid value is exactly 64 lowercase hex characters. Because this field is accepted " + "over the wire but not yet consumed (not forwarded to Mixpanel, not persisted), a " + "malformed value is tolerated: it is coerced to null rather than rejecting the " + "otherwise-valid report." ), ) + @field_validator("api_key_fingerprint", mode="before") + @classmethod + def _tolerate_malformed_fingerprint(cls, value: Any) -> str | None: + """Coerce a malformed, not-yet-consumed fingerprint to ``None`` instead of rejecting. + + The fingerprint is a forward-compatible wire signal that no consumer reads yet, so one + malformed value must not drop an otherwise-valid community report. Any value that is not + ``None`` and not a ``str`` matching the 64-lowercase-hex ``_FINGERPRINT_PATTERN`` (including + non-string junk) is coerced to ``None``, preserving the "present ⇒ valid 64-hex" invariant + for the deferred identity follow-up. ``auth_origin`` stays strictly validated because it is + consumed. + """ + if value is None or (isinstance(value, str) and _FINGERPRINT_PATTERN.fullmatch(value)): + return value + logger.debug("Coercing malformed api_key_fingerprint to None (type=%s)", type(value).__name__) + return None + # --- Models for Pagination --- class NextCallInfo(BaseModel): diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index 771581ad..6db23782 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -258,15 +258,15 @@ def _fingerprint_pro_api_key(key: str) -> str: return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 -def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: +def compute_auth_signals(ctx: Any, include_server_fingerprint: bool = True) -> tuple[AuthOrigin, str | None]: """Derive the ``(auth_origin, api_key_fingerprint)`` pair for *ctx* in one pass. Single source of truth for both signals: each precedence branch returns the origin and the matching fingerprint together, so the two can never disagree (``"client"`` ↔ client hash, ``"server"`` ↔ server hash, ``"none"`` ↔ - ``None``). :func:`compute_auth_origin` and :func:`compute_api_key_fingerprint` - are thin views over this function; callers needing both should call this - directly to avoid extracting the key state twice. + ``None``). :func:`compute_auth_origin` is a thin view over this function; + callers needing both should call this directly to avoid extracting the key + state twice. Never raises (delegates to :func:`extract_client_pro_api_key_from_ctx`, which never raises) and never calls :func:`resolve_pro_api_key` (it reads the @@ -280,6 +280,14 @@ def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: - No client key (absent) → ``("server", )`` when ``config.pro_api_key`` is truthy, otherwise ``("none", None)``. + ``include_server_fingerprint`` gates *only* the server-key hash. Its sole + consumer is the community usage report, so callers pass ``False`` when + community reporting is disabled to skip an SHA-256 that nothing would read + (the server branch then returns ``("server", None)`` — origin unchanged). The + client-key fingerprint is *always* derived under any active sink: it is + deliberate pre-provisioning for the deferred Mixpanel ``distinct_id`` follow-up + that will consume it, so it is never gated here. + The raw key is never returned, logged, or used anywhere but as the hash input. """ @@ -293,6 +301,10 @@ def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: # _Absent — fall back to whether a server key is configured. if config.pro_api_key: + # The server-key fingerprint's only consumer is the community usage + # report; skip the hash entirely when that sink is disabled. + if not include_server_fingerprint: + return "server", None return "server", _fingerprint_pro_api_key(config.pro_api_key) return "none", None @@ -303,21 +315,16 @@ def compute_auth_origin(ctx: Any) -> AuthOrigin: Thin view over :func:`compute_auth_signals` for call sites that consume the origin alone (e.g. the Mixpanel property bag, where the fingerprint is deliberately not emitted). - """ - return compute_auth_signals(ctx)[0] - - -def compute_api_key_fingerprint(ctx: Any) -> str | None: - """Return only the one-way fingerprint of the effective PRO API key for *ctx*. - Thin view over :func:`compute_auth_signals`. Intentionally retained for API - symmetry with :func:`compute_auth_origin`: today's sinks read the fingerprint - via :func:`compute_auth_signals` directly, so this wrapper has no production - caller yet — it is kept on purpose as the ready-made entry point for the - deferred fingerprint consumer (the follow-up that forwards/persists the - fingerprint for unique-user identity). Do not prune as unused. + Because it discards the fingerprint (``[0]``), it passes + ``include_server_fingerprint=False`` to skip the server-key SHA-256 that + would otherwise be computed and thrown away — the origin is identical on + every branch, so this changes nothing observable. Note this only skips the + *server*-key hash: the ``"client"`` branch of :func:`compute_auth_signals` + still hashes the client key (that path is not gated), but it is + ~unreachable here in production and its cost is out of scope. """ - return compute_auth_signals(ctx)[1] + return compute_auth_signals(ctx, include_server_fingerprint=False)[0] # --------------------------------------------------------------------------- diff --git a/blockscout_mcp_server/telemetry.py b/blockscout_mcp_server/telemetry.py index 8768b22b..af9a41e5 100644 --- a/blockscout_mcp_server/telemetry.py +++ b/blockscout_mcp_server/telemetry.py @@ -38,6 +38,12 @@ def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: HTTP mode is off the analytics sink early-returns before its ``ctx`` re-derivation fallback, so a ``None`` origin from this short-circuit is never observed by it. + The server-key fingerprint is gated on the community sink: its only consumer + is the community usage report, so ``include_server_fingerprint`` is passed as + ``not config.disable_community_telemetry`` to skip that SHA-256 when community + reporting is off. The client-key fingerprint is unaffected — it is still + derived for the analytics sink and the deferred identity follow-up. + Never raises: :func:`compute_auth_signals` is defensive today, but the guard is kept so this observability concern can never propagate into the tool body even if that contract later changes. The ``(None, None)`` fallback degrades gracefully @@ -47,7 +53,7 @@ def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: if not analytics.is_http_mode_enabled() and config.disable_community_telemetry: return None, None try: - return compute_auth_signals(ctx) + return compute_auth_signals(ctx, include_server_fingerprint=not config.disable_community_telemetry) except Exception: return None, None diff --git a/tests/api/test_routes.py b/tests/api/test_routes.py index 69fe157c..0def1aef 100644 --- a/tests/api/test_routes.py +++ b/tests/api/test_routes.py @@ -811,8 +811,13 @@ async def test_report_tool_usage_rejects_bad_auth_origin_enum(client: AsyncClien @pytest.mark.asyncio -async def test_report_tool_usage_rejects_malformed_fingerprint(client: AsyncClient): - """A syntactically invalid api_key_fingerprint is rejected with a 422.""" +@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") +async def test_report_tool_usage_tolerates_malformed_fingerprint(mock_track, client: AsyncClient): + """A syntactically invalid api_key_fingerprint does not drop the otherwise-valid report. + + The not-yet-consumed fingerprint is coerced to None by the model, so the report is still + accepted (202) and forwarded once with `api_key_fingerprint is None`. + """ payload = { "tool_name": "dummy", "tool_args": {"a": 1}, @@ -823,7 +828,10 @@ async def test_report_tool_usage_rejects_malformed_fingerprint(client: AsyncClie } headers = {"User-Agent": "BlockscoutMCP/0.0"} response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) - assert response.status_code == 422 + assert response.status_code == 202 + mock_track.assert_called_once() + _, kwargs = mock_track.call_args + assert kwargs["report"].api_key_fingerprint is None @pytest.mark.asyncio diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index ba98502e..045c4819 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -10,13 +10,14 @@ import hashlib from types import SimpleNamespace +from unittest.mock import MagicMock from starlette.datastructures import Headers +from blockscout_mcp_server import pro_api_key_context from blockscout_mcp_server.config import config from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX from blockscout_mcp_server.pro_api_key_context import ( - compute_api_key_fingerprint, compute_auth_origin, compute_auth_signals, ) @@ -111,39 +112,6 @@ def test_auth_origin_feature_disabled_with_no_server_key_is_none(monkeypatch): assert compute_auth_origin(ctx) == "none" -# =========================================================================== -# compute_api_key_fingerprint -# =========================================================================== - - -def test_fingerprint_valid_client_header_matches_expected_hash(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - assert compute_api_key_fingerprint(ctx) == _expected_fingerprint("client-key-123") - - -def test_fingerprint_absent_header_with_server_key_matches_expected_hash(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") - assert compute_api_key_fingerprint(ctx) == _expected_fingerprint("server-key") - - -def test_fingerprint_malformed_header_is_none(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") - assert compute_api_key_fingerprint(ctx) is None - - -def test_fingerprint_absent_header_with_no_server_key_is_none(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") - assert compute_api_key_fingerprint(ctx) is None - - # =========================================================================== # No-fallback precedence (competing server key configured) # =========================================================================== @@ -163,15 +131,6 @@ def test_auth_origin_valid_client_header_wins_over_configured_server_key(monkeyp assert compute_auth_origin(ctx) == "client" -def test_fingerprint_valid_client_header_wins_over_configured_server_key(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - fingerprint = compute_api_key_fingerprint(ctx) - assert fingerprint == _expected_fingerprint("client-key-123") - assert fingerprint != _expected_fingerprint("server-key") - - def test_auth_origin_malformed_client_header_does_not_fall_back_to_server_key(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) @@ -179,13 +138,6 @@ def test_auth_origin_malformed_client_header_does_not_fall_back_to_server_key(mo assert compute_auth_origin(ctx) == "none" -def test_fingerprint_malformed_client_header_does_not_fall_back_to_server_key(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") - assert compute_api_key_fingerprint(ctx) is None - - # =========================================================================== # Fingerprint safety: raw key never leaks; prefix actually participates # =========================================================================== @@ -196,7 +148,7 @@ def test_fingerprint_never_equals_or_contains_raw_key(monkeypatch): monkeypatch.setattr(config, "pro_api_key", "", raising=False) raw_key = "super-secret-client-key-456" ctx = _ctx_with_header(_HEADER_NAME, raw_key) - fingerprint = compute_api_key_fingerprint(ctx) + fingerprint = compute_auth_signals(ctx)[1] assert fingerprint is not None assert fingerprint != raw_key assert raw_key not in fingerprint @@ -208,7 +160,7 @@ def test_fingerprint_prefix_actually_participates(monkeypatch): monkeypatch.setattr(config, "pro_api_key", "", raising=False) raw_key = "client-key-789" ctx = _ctx_with_header(_HEADER_NAME, raw_key) - fingerprint = compute_api_key_fingerprint(ctx) + fingerprint = compute_auth_signals(ctx)[1] unprefixed_hash = hashlib.sha256(raw_key.encode("utf-8")).hexdigest() assert fingerprint != unprefixed_hash @@ -217,11 +169,11 @@ def test_fingerprint_prefix_actually_participates(monkeypatch): # compute_auth_signals — the single source of truth for both signals # =========================================================================== # -# compute_auth_origin / compute_api_key_fingerprint are thin views over this -# function. These cases pin the (origin, fingerprint) pairing per branch so the -# two signals can never silently diverge, and assert the two thin wrappers stay -# in lockstep with the combined result on every branch (a server key is -# configured throughout so a server-fallback regression cannot hide). +# compute_auth_origin is a thin view over this function. These cases pin the +# (origin, fingerprint) pairing per branch so the two signals can never silently +# diverge, and assert the thin wrapper stays in lockstep with the combined +# result on every branch (a server key is configured throughout so a +# server-fallback regression cannot hide). def test_auth_signals_valid_client_returns_client_and_client_hash(monkeypatch): @@ -253,7 +205,7 @@ def test_auth_signals_absent_with_no_server_key_returns_none_and_no_fingerprint( def test_thin_wrappers_match_combined_signals_on_every_branch(monkeypatch): - """compute_auth_origin / compute_api_key_fingerprint must equal the combined pair.""" + """compute_auth_origin must equal the origin of the combined pair.""" monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) for ctx in ( @@ -261,6 +213,62 @@ def test_thin_wrappers_match_combined_signals_on_every_branch(monkeypatch): _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey"), # malformed _ctx_with_header(_HEADER_NAME, ""), # absent -> server fallback ): - origin, fingerprint = compute_auth_signals(ctx) + origin, _fingerprint = compute_auth_signals(ctx) assert compute_auth_origin(ctx) == origin - assert compute_api_key_fingerprint(ctx) == fingerprint + + +# =========================================================================== +# include_server_fingerprint gate — server hash only when a consumer exists +# =========================================================================== +# +# The server-key fingerprint's only consumer is the community usage report, so +# resolve_auth_signals passes include_server_fingerprint=False when community +# reporting is disabled. Only the absent-client / server-key branch is gated; +# the client-key path is deliberately never gated (pre-provisioning for the +# deferred distinct_id follow-up). + + +def test_auth_signals_absent_with_server_key_skips_server_hash_when_gated(monkeypatch): + """Absent client key + server key + include_server_fingerprint=False -> ("server", None). + + Origin stays "server"; only the SHA-256 is skipped because nothing consumes it. + """ + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "") + assert compute_auth_signals(ctx, include_server_fingerprint=False) == ("server", None) + + +def test_auth_signals_valid_client_hash_not_gated_by_server_flag(monkeypatch): + """A valid client key still yields ("client", ) even with the flag False. + + include_server_fingerprint gates only the server-key branch; the client-key + fingerprint is preserved as pre-provisioning for the deferred identity work. + """ + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + assert compute_auth_signals(ctx, include_server_fingerprint=False) == ( + "client", + _expected_fingerprint("client-key-123"), + ) + + +def test_compute_auth_origin_skips_server_hash_on_server_branch(monkeypatch): + """compute_auth_origin returns "server" WITHOUT computing the server-key SHA-256. + + Pins the optimization: because ``compute_auth_origin`` discards the + fingerprint, it passes ``include_server_fingerprint=False``, so on the + absent-client / configured-server-key branch it must derive the origin + without ever calling ``_fingerprint_pro_api_key``. If a future change drops + the ``False`` argument, the spy below records a call and this test fails. + """ + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + + fingerprint_spy = MagicMock(side_effect=pro_api_key_context._fingerprint_pro_api_key) + monkeypatch.setattr(pro_api_key_context, "_fingerprint_pro_api_key", fingerprint_spy) + + ctx = _ctx_with_header(_HEADER_NAME, "") # absent client key -> server fallback + assert compute_auth_origin(ctx) == "server" + fingerprint_spy.assert_not_called() diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 0c10af33..ec94020c 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -1,7 +1,9 @@ # SPDX-License-Identifier: LicenseRef-Blockscout +from types import SimpleNamespace from unittest.mock import ANY, AsyncMock, MagicMock, patch import pytest +from starlette.datastructures import Headers from blockscout_mcp_server import analytics, telemetry from blockscout_mcp_server.config import config @@ -32,7 +34,11 @@ def test_resolve_auth_signals_short_circuits_when_all_telemetry_disabled(monkeyp def test_resolve_auth_signals_derives_when_community_enabled(monkeypatch): - """Community telemetry enabled (HTTP mode off) still needs the signals -> derivation runs.""" + """Community telemetry enabled (HTTP mode off) still needs the signals -> derivation runs. + + The community sink consumes the server-key fingerprint, so the server hash is + requested (``include_server_fingerprint=True``). + """ monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) analytics.set_http_mode(False) compute_spy = MagicMock(return_value=("server", "d" * 64)) @@ -40,11 +46,15 @@ def test_resolve_auth_signals_derives_when_community_enabled(monkeypatch): ctx = object() assert telemetry.resolve_auth_signals(ctx) == ("server", "d" * 64) - compute_spy.assert_called_once_with(ctx) + compute_spy.assert_called_once_with(ctx, include_server_fingerprint=True) def test_resolve_auth_signals_derives_in_http_mode_even_if_community_disabled(monkeypatch): - """HTTP mode on feeds the analytics sink, so derivation runs even when community is disabled.""" + """HTTP mode on feeds the analytics sink, so derivation runs even when community is disabled. + + Community is off, so the server-key fingerprint has no consumer: the flag is + threaded through as ``include_server_fingerprint=False``. + """ monkeypatch.setattr(config, "disable_community_telemetry", True, raising=False) analytics.set_http_mode(True) try: @@ -53,7 +63,28 @@ def test_resolve_auth_signals_derives_in_http_mode_even_if_community_disabled(mo ctx = object() assert telemetry.resolve_auth_signals(ctx) == ("client", "e" * 64) - compute_spy.assert_called_once_with(ctx) + compute_spy.assert_called_once_with(ctx, include_server_fingerprint=False) + finally: + analytics.set_http_mode(False) + + +def test_resolve_auth_signals_skips_server_fingerprint_when_community_disabled(monkeypatch): + """End-to-end gate: absent client key + server key + HTTP on + community off -> ("server", None). + + Uses the real compute_auth_signals (no spy) to prove the flag is threaded + through and the server-key SHA-256 is skipped, while auth_origin stays "server". + """ + monkeypatch.setattr(config, "disable_community_telemetry", True, raising=False) + monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) + monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + analytics.set_http_mode(True) + try: + ctx = SimpleNamespace( + request_context=SimpleNamespace( + request=SimpleNamespace(headers=Headers(headers={"Blockscout-MCP-Pro-Api-Key": ""})) + ) + ) + assert telemetry.resolve_auth_signals(ctx) == ("server", None) finally: analytics.set_http_mode(False) diff --git a/tests/test_tool_usage_report.py b/tests/test_tool_usage_report.py index d04dc17a..bdcba77c 100644 --- a/tests/test_tool_usage_report.py +++ b/tests/test_tool_usage_report.py @@ -76,29 +76,49 @@ def test_explicit_null_auth_origin_with_null_fingerprint_round_trips(): assert report.api_key_fingerprint is None -def test_api_key_fingerprint_rejects_too_short_value(): - """A malformed (too short) api_key_fingerprint raises a pydantic ValidationError.""" - with pytest.raises(ValidationError): - ToolUsageReport(**_base_payload(api_key_fingerprint="abc")) +def test_api_key_fingerprint_tolerates_too_short_value(): + """A malformed (too short) api_key_fingerprint is tolerated: coerced to None, not rejected. + The fingerprint is a not-yet-consumed forward-compat field, so a malformed value must not + drop the otherwise-valid report. + """ + report = ToolUsageReport(**_base_payload(api_key_fingerprint="abc")) + + assert report.api_key_fingerprint is None -def test_api_key_fingerprint_rejects_non_hex_value(): - """A 64-character but non-hex api_key_fingerprint raises a pydantic ValidationError.""" - with pytest.raises(ValidationError): - ToolUsageReport(**_base_payload(api_key_fingerprint="z" * 64)) +def test_api_key_fingerprint_tolerates_non_hex_value(): + """A 64-character but non-hex api_key_fingerprint is coerced to None rather than rejected.""" + report = ToolUsageReport(**_base_payload(api_key_fingerprint="z" * 64)) -def test_api_key_fingerprint_rejects_uppercase_hex_value(): - """A 64-character uppercase-hex api_key_fingerprint raises a pydantic ValidationError. + assert report.api_key_fingerprint is None + + +def test_api_key_fingerprint_tolerates_uppercase_hex_value(): + """A 64-character uppercase-hex api_key_fingerprint is coerced to None rather than rejected. - The pattern `^[0-9a-f]{64}$` only accepts lowercase hex, matching the lowercase output of - `hashlib.sha256(...).hexdigest()`. + Only lowercase hex is valid (matching the lowercase output of + `hashlib.sha256(...).hexdigest()`), so uppercase is treated as malformed and tolerated. """ - with pytest.raises(ValidationError): - ToolUsageReport(**_base_payload(api_key_fingerprint="A" * 64)) + report = ToolUsageReport(**_base_payload(api_key_fingerprint="A" * 64)) + + assert report.api_key_fingerprint is None -def test_api_key_fingerprint_rejects_over_length_value(): - """An over-length api_key_fingerprint raises a pydantic ValidationError.""" - with pytest.raises(ValidationError): - ToolUsageReport(**_base_payload(api_key_fingerprint="a" * 65)) +def test_api_key_fingerprint_tolerates_over_length_value(): + """An over-length api_key_fingerprint is coerced to None rather than rejected.""" + report = ToolUsageReport(**_base_payload(api_key_fingerprint="a" * 65)) + + assert report.api_key_fingerprint is None + + +@pytest.mark.parametrize("junk", [123, ["a" * 64], {"fingerprint": "a" * 64}, 45.6, True]) +def test_api_key_fingerprint_tolerates_non_string_junk(junk): + """A non-string junk api_key_fingerprint (int/list/dict/float/bool) is coerced to None. + + The `mode="before"` validator guards with `isinstance(value, str)` before the regex, so + non-string input never reaches the pattern and is tolerated as None rather than raising. + """ + report = ToolUsageReport(**_base_payload(api_key_fingerprint=junk)) + + assert report.api_key_fingerprint is None From db9cd27c685aaf188c8b0a551d7d1e99d5af7b50 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 19:48:28 -0600 Subject: [PATCH 14/26] test: pin disable_community_telemetry in auth-signal tests Make the four new auth-origin/fingerprint tests hermetic to the BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY environment variable. With community telemetry disabled and HTTP mode off, resolve_auth_signals short-circuits to (None, None), so an ambient env/.env value would fail the auth_origin assertions even though production is correct. Co-Authored-By: Claude Opus 4.8 --- tests/test_observability.py | 3 +++ tests/tools/test_decorators.py | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/tests/test_observability.py b/tests/test_observability.py index 860ee024..1fb39274 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -124,6 +124,9 @@ def test_fan_out_both_sinks_called(): def test_community_sink_forwards_auth_origin_and_fingerprint(monkeypatch): """send_community_resource_report receives auth_origin/api_key_fingerprint derived from ctx.""" + # Keep community telemetry enabled so telemetry.resolve_auth_signals derives the signals instead + # of short-circuiting to (None, None) when BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY is set ambiently. + monkeypatch.setattr("blockscout_mcp_server.telemetry.config.disable_community_telemetry", False, raising=False) monkeypatch.setattr("blockscout_mcp_server.pro_api_key_context.config.pro_api_key", "server-key", raising=False) ctx = _make_ctx() diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index 63af2d4e..259e22a1 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -48,6 +48,11 @@ def fake_track(ctx, name, args, client_meta=None, auth_origin=None): # type: ig async def dummy_tool(a: int, ctx: Context) -> int: return a + # Pin community telemetry on so resolve_auth_signals derives instead of short-circuiting to + # (None, None): with community disabled (e.g. BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY set in the + # env or a local .env) and HTTP mode off, no sink would consume the signals and auth_origin + # would be None regardless of the header — making the assertion below untestable. + monkeypatch.setattr(server_config, "disable_community_telemetry", False, raising=False) # A valid client PRO API key header makes the derived origin deterministic ("client"). # Pinning the exact value is what gives the assertion teeth: `in ("client","server","none")` # is vacuous because every AuthOrigin member trivially satisfies it, so it would pass even @@ -147,6 +152,9 @@ async def dummy_tool(a: int, ctx: Context) -> int: new_callable=AsyncMock, ) async def test_decorator_reports_telemetry(mock_report, monkeypatch, mock_ctx: Context) -> None: + # Keep community telemetry enabled so resolve_auth_signals derives auth_origin instead of + # short-circuiting to (None, None) when BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY is set ambiently. + monkeypatch.setattr(server_config, "disable_community_telemetry", False, raising=False) monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) @log_tool_invocation @@ -175,6 +183,9 @@ async def dummy_tool(a: int, ctx: Context) -> int: ) async def test_decorator_reports_telemetry_with_client_key(mock_report, monkeypatch, mock_ctx: Context) -> None: """A client-supplied PRO API key header is forwarded as a non-reversible fingerprint.""" + # Keep community telemetry enabled so resolve_auth_signals derives auth_origin instead of + # short-circuiting to (None, None) when BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY is set ambiently. + monkeypatch.setattr(server_config, "disable_community_telemetry", False, raising=False) monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) raw_key = "super-secret-client-key" From 376517dbb17ddd110e07c0726a115d59e151b5d2 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 20:06:47 -0600 Subject: [PATCH 15/26] Address review: drop harmful auth_origin ctx re-derivation fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The analytics sink's `auth_origin if not None else compute_auth_origin(ctx)` fallback was unreachable when HTTP mode is off (track_tool_invocation early-returns) and actively harmful in the one reachable case: when resolve_auth_signals swallowed a compute_auth_signals exception and returned None, re-deriving via compute_auth_origin(ctx) re-ran the same failing chain, was eaten by the except, and dropped the whole Mixpanel event — contradicting the "graceful degradation" the docstrings promised. Map None to AUTH_ORIGIN_UNKNOWN (mirroring track_community_usage) so the event is still sent, and remove the now-dead compute_auth_origin helper, its import, tests, and docs. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 2 +- blockscout_mcp_server/analytics.py | 17 ++- blockscout_mcp_server/observability.py | 4 +- blockscout_mcp_server/pro_api_key_context.py | 24 +--- blockscout_mcp_server/telemetry.py | 8 +- tests/test_analytics.py | 33 +++-- .../test_pro_api_key_context_auth_signals.py | 129 ++---------------- tests/tools/test_decorators.py | 24 ++-- 8 files changed, 61 insertions(+), 180 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ae582840..cd0aba55 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -398,7 +398,7 @@ mcp-server/ * **`pro_api_key_context.py`**: * Owns request-scoped resolution of a client-supplied Blockscout PRO API key, kept separate from logging/observability. * Provides a `ContextVar` of the per-request client-key state, a normalization/validation helper, `extract_client_pro_api_key_from_ctx()`, `resolve_pro_api_key()` (precedence: valid client key → server key → not-configured error; malformed client key → terminal error, no fallback), and the `@pro_api_key_scope` decorator. - * Also provides the `ctx`-derived helpers `compute_auth_signals()` (community-telemetry path) and `compute_auth_origin()` (analytics path), which run outside `@pro_api_key_scope`. + * Also provides the `ctx`-derived helper `compute_auth_signals()` (used by the analytics and community-telemetry paths), which runs outside `@pro_api_key_scope`. * Honored for any HTTP request that carries the configured header (MCP-over-HTTP or REST); the key is never logged or placed in cache keys. * Also defines the per-invocation credit-tracking symbols: `CreditSink`, the `_credit_sink` `ContextVar`, and the `@pro_api_credit_scope` decorator (a sibling of `@pro_api_key_scope`). * **`cache.py`**: diff --git a/blockscout_mcp_server/analytics.py b/blockscout_mcp_server/analytics.py index ac2b1f3a..d52359f0 100644 --- a/blockscout_mcp_server/analytics.py +++ b/blockscout_mcp_server/analytics.py @@ -39,7 +39,6 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: # noqa: D401 - simple pl from blockscout_mcp_server.config import config from blockscout_mcp_server.constants import AUTH_ORIGIN_UNKNOWN, RESOURCE_READ_EVENT, AuthOrigin from blockscout_mcp_server.models import ToolUsageReport -from blockscout_mcp_server.pro_api_key_context import compute_auth_origin logger = logging.getLogger(__name__) @@ -196,11 +195,15 @@ def track_tool_invocation( ) -> None: """Track a tool invocation in Mixpanel, if enabled and in HTTP mode. - ``auth_origin`` may be supplied pre-computed by the caller — the tool - decorator already derives the auth signals once per invocation (and reuses - them for the community report), so it threads the origin in here to avoid a - second ``ctx`` extraction and SHA-256 hash on the hot path. When omitted - (e.g. direct callers) it is derived from ``ctx`` here, preserving behavior. + ``auth_origin`` is the pre-computed origin threaded in by the caller — the + observability paths derive the auth signals once per invocation (via + :func:`blockscout_mcp_server.telemetry.resolve_auth_signals`) and reuse them + for both this sink and the community report, so the origin is never + re-derived here. A ``None`` value — which ``resolve_auth_signals`` yields + when derivation is skipped or degrades — is recorded as + ``AUTH_ORIGIN_UNKNOWN``, mirroring :func:`track_community_usage`. Re-deriving + from ``ctx`` at this point would re-run the very computation that just failed + and lose the whole event, so it is deliberately avoided. """ if not _is_http_mode_enabled: return @@ -234,7 +237,7 @@ def track_tool_invocation( "tool_args": tool_args, "protocol_version": protocol_version, "source": _determine_call_source(ctx), - "auth_origin": auth_origin if auth_origin is not None else compute_auth_origin(ctx), + "auth_origin": auth_origin if auth_origin is not None else AUTH_ORIGIN_UNKNOWN, } meta = {"ip": ip} if ip else None diff --git a/blockscout_mcp_server/observability.py b/blockscout_mcp_server/observability.py index 03260abf..7da5023c 100644 --- a/blockscout_mcp_server/observability.py +++ b/blockscout_mcp_server/observability.py @@ -73,8 +73,8 @@ def log_resource_read(uri: Any, ctx: Any) -> None: # sinks below (mirrors @log_tool_invocation). telemetry.resolve_auth_signals # centralizes the single ctx extraction + SHA-256, the defensive guard, and the # all-telemetry-disabled short-circuit shared verbatim with the tool path. The - # (None, None) fallback degrades gracefully — the analytics sink re-derives the - # origin from ctx, the report omits the hash. + # (None, None) fallback degrades gracefully — the analytics sink records the + # origin as AUTH_ORIGIN_UNKNOWN, the report omits the hash. auth_origin, api_key_fingerprint = telemetry.resolve_auth_signals(ctx) # Step 2 — direct analytics sink (self-gating, synchronous). diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index 6db23782..9b815e5a 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -264,9 +264,9 @@ def compute_auth_signals(ctx: Any, include_server_fingerprint: bool = True) -> t Single source of truth for both signals: each precedence branch returns the origin and the matching fingerprint together, so the two can never disagree (``"client"`` ↔ client hash, ``"server"`` ↔ server hash, ``"none"`` ↔ - ``None``). :func:`compute_auth_origin` is a thin view over this function; - callers needing both should call this directly to avoid extracting the key - state twice. + ``None``). Callers that need only the origin can discard the fingerprint + (``[0]``) and pass ``include_server_fingerprint=False`` to skip the + server-key SHA-256 that nothing would then read. Never raises (delegates to :func:`extract_client_pro_api_key_from_ctx`, which never raises) and never calls :func:`resolve_pro_api_key` (it reads the @@ -309,24 +309,6 @@ def compute_auth_signals(ctx: Any, include_server_fingerprint: bool = True) -> t return "none", None -def compute_auth_origin(ctx: Any) -> AuthOrigin: - """Return only the authorization origin for *ctx*. - - Thin view over :func:`compute_auth_signals` for call sites that consume the - origin alone (e.g. the Mixpanel property bag, where the fingerprint is - deliberately not emitted). - - Because it discards the fingerprint (``[0]``), it passes - ``include_server_fingerprint=False`` to skip the server-key SHA-256 that - would otherwise be computed and thrown away — the origin is identical on - every branch, so this changes nothing observable. Note this only skips the - *server*-key hash: the ``"client"`` branch of :func:`compute_auth_signals` - still hashes the client key (that path is not gated), but it is - ~unreachable here in production and its cost is out of scope. - """ - return compute_auth_signals(ctx, include_server_fingerprint=False)[0] - - # --------------------------------------------------------------------------- # 5. Resolver — applies client-key → server-key precedence # --------------------------------------------------------------------------- diff --git a/blockscout_mcp_server/telemetry.py b/blockscout_mcp_server/telemetry.py index af9a41e5..f1912590 100644 --- a/blockscout_mcp_server/telemetry.py +++ b/blockscout_mcp_server/telemetry.py @@ -35,8 +35,8 @@ def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: deliberately a *superset* of the precise per-sink gates (it may still derive in a rare config where only the suppressed sink would have run); erring toward deriving keeps this cheap check independent of the sinks' internal logic. When - HTTP mode is off the analytics sink early-returns before its ``ctx`` re-derivation - fallback, so a ``None`` origin from this short-circuit is never observed by it. + HTTP mode is off the analytics sink early-returns anyway, so a ``None`` origin + from this short-circuit never reaches its property bag. The server-key fingerprint is gated on the community sink: its only consumer is the community usage report, so ``include_server_fingerprint`` is passed as @@ -47,8 +47,8 @@ def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: Never raises: :func:`compute_auth_signals` is defensive today, but the guard is kept so this observability concern can never propagate into the tool body even if that contract later changes. The ``(None, None)`` fallback degrades gracefully - — the analytics sink re-derives the origin from ``ctx``, the community report - omits the hash. + — the analytics sink records the origin as ``AUTH_ORIGIN_UNKNOWN`` (it never + re-derives from ``ctx``), the community report omits the hash. """ if not analytics.is_http_mode_enabled() and config.disable_community_telemetry: return None, None diff --git a/tests/test_analytics.py b/tests/test_analytics.py index bed9cfaa..f243804a 100644 --- a/tests/test_analytics.py +++ b/tests/test_analytics.py @@ -145,10 +145,9 @@ def test_tracks_with_intermediary_no_client_or_user_agent(monkeypatch): assert args[2]["client_name"] == "N/A/ClaudeDesktop" -def test_tracks_auth_origin_server_when_no_client_header_and_server_key(monkeypatch): - """No client-supplied key, but a server key is configured -> auth_origin == 'server'.""" +def test_tracks_threaded_auth_origin_into_property_bag(monkeypatch): + """The pre-computed ``auth_origin`` threaded by the caller reaches the property bag verbatim.""" monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) - monkeypatch.setattr(server_config, "pro_api_key", "server-secret", raising=False) headers = {"x-forwarded-for": "203.0.113.5", "user-agent": "pytest-UA"} req = DummyRequest(headers=headers) ctx = DummyCtx(request=req, client_name="clientA", client_version="1.0.0") @@ -156,15 +155,20 @@ def test_tracks_auth_origin_server_when_no_client_header_and_server_key(monkeypa mp_instance = MagicMock() mp_cls.return_value = mp_instance analytics.set_http_mode(True) - analytics.track_tool_invocation(ctx, "tool_name", {"x": 2}) + analytics.track_tool_invocation(ctx, "tool_name", {"x": 2}, auth_origin="server") args, _ = mp_instance.track.call_args assert args[2]["auth_origin"] == "server" -def test_tracks_auth_origin_none_when_no_client_header_and_no_server_key(monkeypatch): - """Neither a client-supplied key nor a server key -> auth_origin == 'none'.""" +def test_tracks_auth_origin_unknown_when_not_threaded(monkeypatch): + """When no ``auth_origin`` is threaded (``None``), the sink records the ``unknown`` sentinel. + + The sink never re-derives the origin from ``ctx`` — doing so would re-run the + computation that ``telemetry.resolve_auth_signals`` already guarded and, if it + fails, drop the whole Mixpanel event. Mirrors ``track_community_usage``. + """ monkeypatch.setattr(server_config, "mixpanel_token", "test-token", raising=False) - monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) + monkeypatch.setattr(server_config, "pro_api_key", "server-secret", raising=False) headers = {"x-forwarded-for": "203.0.113.5", "user-agent": "pytest-UA"} req = DummyRequest(headers=headers) ctx = DummyCtx(request=req, client_name="clientA", client_version="1.0.0") @@ -174,7 +178,7 @@ def test_tracks_auth_origin_none_when_no_client_header_and_no_server_key(monkeyp analytics.set_http_mode(True) analytics.track_tool_invocation(ctx, "tool_name", {"x": 2}) args, _ = mp_instance.track.call_args - assert args[2]["auth_origin"] == "none" + assert args[2]["auth_origin"] == "unknown" def test_track_event_tracks_when_enabled(monkeypatch): @@ -225,7 +229,7 @@ def test_pro_api_key_not_in_analytics_payload(monkeypatch): mp_instance = MagicMock() mp_cls.return_value = mp_instance analytics.set_http_mode(True) - analytics.track_tool_invocation(ctx, "some_tool", {"x": 1}) + analytics.track_tool_invocation(ctx, "some_tool", {"x": 1}, auth_origin="client") assert mp_instance.track.called call_args = mp_instance.track.call_args @@ -242,7 +246,7 @@ def test_pro_api_key_not_in_analytics_payload(monkeypatch): assert client_key not in str(properties) assert client_key not in str(kwargs) - # The client supplied a valid header, so auth_origin must be 'client'. + # The caller threaded auth_origin='client'; it reaches the bag but the key never does. assert properties.get("auth_origin") == "client" assert "api_key_fingerprint" not in properties @@ -273,7 +277,7 @@ def test_pro_api_key_not_in_analytics_payload_rest_source(monkeypatch): mp_instance = MagicMock() mp_cls.return_value = mp_instance analytics.set_http_mode(True) - analytics.track_tool_invocation(ctx, "some_tool", {"x": 1}) + analytics.track_tool_invocation(ctx, "some_tool", {"x": 1}, auth_origin="client") assert mp_instance.track.called call_args = mp_instance.track.call_args @@ -293,7 +297,7 @@ def test_pro_api_key_not_in_analytics_payload_rest_source(monkeypatch): # Confirm the source is correctly reported as 'rest' assert properties.get("source") == "rest" - # The client supplied a valid header, so auth_origin must be 'client'. + # The caller threaded auth_origin='client'; it reaches the bag but the key never does. assert properties.get("auth_origin") == "client" assert "api_key_fingerprint" not in properties @@ -441,6 +445,7 @@ def test_track_resource_read_emits_correct_event(monkeypatch): ctx, uri, client_meta=ClientMeta(name="clientA", version="1.0.0", protocol="2024-11-05", user_agent="pytest-UA"), + auth_origin="client", ) mp_instance.track.assert_called_once() args, kwargs = mp_instance.track.call_args @@ -452,6 +457,6 @@ def test_track_resource_read_emits_correct_event(monkeypatch): assert properties["protocol_version"] == "2024-11-05" assert properties["ip"] == "203.0.113.5" assert "source" in properties - # track_resource_read delegates to track_tool_invocation, so a header-present - # context yields 'client' for auth_origin, inherited automatically. + # track_resource_read threads the caller's auth_origin straight through to + # track_tool_invocation, so the value provided here reaches the property bag. assert properties["auth_origin"] == "client" diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index 045c4819..8b60d40f 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -17,10 +17,7 @@ from blockscout_mcp_server import pro_api_key_context from blockscout_mcp_server.config import config from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX -from blockscout_mcp_server.pro_api_key_context import ( - compute_auth_origin, - compute_auth_signals, -) +from blockscout_mcp_server.pro_api_key_context import compute_auth_signals _HEADER_NAME = "Blockscout-MCP-Pro-Api-Key" @@ -52,92 +49,6 @@ def _expected_fingerprint(key: str) -> str: return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 -# =========================================================================== -# compute_auth_origin -# =========================================================================== - - -def test_auth_origin_valid_client_header_is_client(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - assert compute_auth_origin(ctx) == "client" - - -def test_auth_origin_malformed_header_is_none(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") - assert compute_auth_origin(ctx) == "none" - - -def test_auth_origin_malformed_over_length_header_is_none(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_malformed_header(_HEADER_NAME, "a" * 257) - assert compute_auth_origin(ctx) == "none" - - -def test_auth_origin_absent_header_with_server_key_is_server(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") - assert compute_auth_origin(ctx) == "server" - - -def test_auth_origin_absent_header_with_no_server_key_is_none(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") - assert compute_auth_origin(ctx) == "none" - - -def test_auth_origin_feature_disabled_with_server_key_is_server(monkeypatch): - """Header feature disabled (empty header config) + server key configured -> server. - - "Feature disabled" alone does not force "none" -- a disabled header feature - makes the extractor return _Absent, which falls through to the server-key - branch just like a genuinely absent header. - """ - monkeypatch.setattr(config, "pro_api_key_header", "", raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - assert compute_auth_origin(ctx) == "server" - - -def test_auth_origin_feature_disabled_with_no_server_key_is_none(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", "", raising=False) - monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - assert compute_auth_origin(ctx) == "none" - - -# =========================================================================== -# No-fallback precedence (competing server key configured) -# =========================================================================== -# -# These cases set config.pro_api_key = "server-key" so a server-fallback -# regression cannot hide: a valid client header must still win, and a -# malformed client header must still yield no usable key (no silent fallback -# to the configured server key). Mirrors the precedence already enforced by -# resolve_pro_api_key(). Kept distinct from the absent-header cases above, -# which deliberately exercise the server branch. - - -def test_auth_origin_valid_client_header_wins_over_configured_server_key(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - assert compute_auth_origin(ctx) == "client" - - -def test_auth_origin_malformed_client_header_does_not_fall_back_to_server_key(monkeypatch): - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") - assert compute_auth_origin(ctx) == "none" - - # =========================================================================== # Fingerprint safety: raw key never leaks; prefix actually participates # =========================================================================== @@ -169,11 +80,11 @@ def test_fingerprint_prefix_actually_participates(monkeypatch): # compute_auth_signals — the single source of truth for both signals # =========================================================================== # -# compute_auth_origin is a thin view over this function. These cases pin the -# (origin, fingerprint) pairing per branch so the two signals can never silently -# diverge, and assert the thin wrapper stays in lockstep with the combined -# result on every branch (a server key is configured throughout so a -# server-fallback regression cannot hide). +# These cases pin the (origin, fingerprint) pairing per branch so the two signals +# can never silently diverge. A server key is configured throughout so a +# server-fallback regression cannot hide: a valid client header must still win and +# a malformed one must still yield no usable key (no silent fallback to the server +# key), mirroring the precedence enforced by resolve_pro_api_key(). def test_auth_signals_valid_client_returns_client_and_client_hash(monkeypatch): @@ -204,19 +115,6 @@ def test_auth_signals_absent_with_no_server_key_returns_none_and_no_fingerprint( assert compute_auth_signals(ctx) == ("none", None) -def test_thin_wrappers_match_combined_signals_on_every_branch(monkeypatch): - """compute_auth_origin must equal the origin of the combined pair.""" - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - for ctx in ( - _ctx_with_header(_HEADER_NAME, "client-key-123"), # valid client - _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey"), # malformed - _ctx_with_header(_HEADER_NAME, ""), # absent -> server fallback - ): - origin, _fingerprint = compute_auth_signals(ctx) - assert compute_auth_origin(ctx) == origin - - # =========================================================================== # include_server_fingerprint gate — server hash only when a consumer exists # =========================================================================== @@ -254,14 +152,13 @@ def test_auth_signals_valid_client_hash_not_gated_by_server_flag(monkeypatch): ) -def test_compute_auth_origin_skips_server_hash_on_server_branch(monkeypatch): - """compute_auth_origin returns "server" WITHOUT computing the server-key SHA-256. +def test_compute_auth_signals_skips_server_hash_when_gated_off(monkeypatch): + """With ``include_server_fingerprint=False`` the server branch derives the origin + WITHOUT computing the server-key SHA-256. - Pins the optimization: because ``compute_auth_origin`` discards the - fingerprint, it passes ``include_server_fingerprint=False``, so on the - absent-client / configured-server-key branch it must derive the origin - without ever calling ``_fingerprint_pro_api_key``. If a future change drops - the ``False`` argument, the spy below records a call and this test fails. + Pins the optimization: on the absent-client / configured-server-key branch, + ``_fingerprint_pro_api_key`` must never be called when the flag is False. If a + future change drops the gate, the spy below records a call and this test fails. """ monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) @@ -270,5 +167,5 @@ def test_compute_auth_origin_skips_server_hash_on_server_branch(monkeypatch): monkeypatch.setattr(pro_api_key_context, "_fingerprint_pro_api_key", fingerprint_spy) ctx = _ctx_with_header(_HEADER_NAME, "") # absent client key -> server fallback - assert compute_auth_origin(ctx) == "server" + assert compute_auth_signals(ctx, include_server_fingerprint=False) == ("server", None) fingerprint_spy.assert_not_called() diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index 259e22a1..7ca4cafe 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -20,7 +20,6 @@ from blockscout_mcp_server.config import config as server_config from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX from blockscout_mcp_server.pro_api_key_context import ( - compute_auth_origin, pro_api_key_scope, resolve_pro_api_key, ) @@ -219,23 +218,19 @@ async def test_decorator_derives_auth_signals_once_and_threads_to_both_sinks( ctx extraction / SHA-256 on the hot path (regression guard for the de-dup fix). This drives the *real* analytics sink (only the Mixpanel client is mocked) and spies on the - real derivation helpers rather than mocking them away. That is what makes the guard - meaningful: if the decorator stopped threading ``auth_origin`` into ``track_tool_invocation``, - the analytics sink's ``compute_auth_origin(ctx)`` fallback would fire — and - ``origin_fallback_spy.assert_not_called()`` would catch it. Mocking the derivation - away (the previous approach) made the "derived once" claim structurally true by construction - instead of observing it. + real derivation helper rather than mocking it away. That is what makes the guard meaningful: + the origin is derived exactly once, by the decorator, and the identical value is observed in + the Mixpanel property bag — proving it was threaded through rather than recomputed by the sink. + Mocking the derivation away (the previous approach) made the "derived once" claim structurally + true by construction instead of observing it. """ # Spy on the single derivation point — the shared telemetry.resolve_auth_signals helper the # decorator delegates to — while keeping the real implementation. signals_spy = MagicMock(side_effect=resolve_auth_signals) monkeypatch.setattr("blockscout_mcp_server.telemetry.resolve_auth_signals", signals_spy) - # Spy on the analytics fallback; it must NEVER run because the origin is threaded in. - origin_fallback_spy = MagicMock(side_effect=compute_auth_origin) - monkeypatch.setattr("blockscout_mcp_server.analytics.compute_auth_origin", origin_fallback_spy) - # Drive the real analytics sink with a mocked Mixpanel client so the property bag (and thus - # the fallback branch) is genuinely exercised rather than structurally bypassed. + # Drive the real analytics sink with a mocked Mixpanel client so the property bag is + # genuinely exercised rather than structurally bypassed. mock_mp = MagicMock() monkeypatch.setattr("blockscout_mcp_server.analytics._is_http_mode_enabled", True, raising=False) monkeypatch.setattr("blockscout_mcp_server.analytics._get_mixpanel_client", lambda: mock_mp) @@ -257,10 +252,9 @@ async def dummy_tool(a: int, ctx: Context) -> int: # Derived exactly once by the decorator... signals_spy.assert_called_once() - # ...and the analytics sink reused the threaded origin instead of re-deriving it. - origin_fallback_spy.assert_not_called() - # The same origin reached the Mixpanel property bag (3rd positional arg of mp.track). + # ...and the same origin reached the Mixpanel property bag (3rd positional arg of mp.track), + # confirming the sink reused the threaded value instead of re-deriving it. mock_mp.track.assert_called_once() props = mock_mp.track.call_args.args[2] assert props["auth_origin"] == "client" From 40dc2cd2eb921eba3313825bde274a41c71318d0 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 20:20:29 -0600 Subject: [PATCH 16/26] Address review: memoize server-key fingerprint, drop include_server_fingerprint The include_server_fingerprint gate mirrored only one of send_community_usage_report's two suppression conditions: it skipped the server-key SHA-256 on disable_community_telemetry but not on the (is_http_mode_enabled() and mixpanel_token) early-return. On the official server (HTTP + Mixpanel token + community telemetry enabled + server key set) the hash was therefore recomputed on every tool call / resource read for a community report that is guaranteed never to send. Since config.pro_api_key is fixed for the process lifetime, memoize its fingerprint once (functools.lru_cache) and remove the include_server_fingerprint parameter, its branch in compute_auth_signals, the resolve_auth_signals gate argument, and the flag's docstring sections. Client keys stay on the un-cached path so a per-request secret is never retained. Replace the flag/spy tests with memoization coverage and repurpose the telemetry end-to-end test to assert the full ("server", ) pair. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/pro_api_key_context.py | 40 +++++++----- blockscout_mcp_server/telemetry.py | 13 ++-- .../test_pro_api_key_context_auth_signals.py | 64 +++++++++---------- tests/test_telemetry.py | 26 ++++---- 4 files changed, 76 insertions(+), 67 deletions(-) diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index 9b815e5a..dd32bee3 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -258,15 +258,32 @@ def _fingerprint_pro_api_key(key: str) -> str: return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 -def compute_auth_signals(ctx: Any, include_server_fingerprint: bool = True) -> tuple[AuthOrigin, str | None]: +@functools.lru_cache(maxsize=1) +def _server_api_key_fingerprint(key: str) -> str: + """Return the fingerprint of the configured server key, memoized per process. + + ``config.pro_api_key`` is fixed for the process lifetime, so its SHA-256 is a + hash of a constant. Memoizing it means the observability hot path (every tool + call / resource read) reuses the one digest instead of re-hashing the same + value each time. ``maxsize=1`` keeps a single entry: production never varies + the key, and tests that monkeypatch ``config.pro_api_key`` cache-miss on the + new value and evict the stale one. + + Only the *server* key is memoized. Client keys are per-request secrets, so they + stay on the un-cached :func:`_fingerprint_pro_api_key` path and are never + retained in this cache. + """ + return _fingerprint_pro_api_key(key) + + +def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: """Derive the ``(auth_origin, api_key_fingerprint)`` pair for *ctx* in one pass. Single source of truth for both signals: each precedence branch returns the origin and the matching fingerprint together, so the two can never disagree (``"client"`` ↔ client hash, ``"server"`` ↔ server hash, ``"none"`` ↔ ``None``). Callers that need only the origin can discard the fingerprint - (``[0]``) and pass ``include_server_fingerprint=False`` to skip the - server-key SHA-256 that nothing would then read. + (``[1]``). Never raises (delegates to :func:`extract_client_pro_api_key_from_ctx`, which never raises) and never calls :func:`resolve_pro_api_key` (it reads the @@ -280,13 +297,10 @@ def compute_auth_signals(ctx: Any, include_server_fingerprint: bool = True) -> t - No client key (absent) → ``("server", )`` when ``config.pro_api_key`` is truthy, otherwise ``("none", None)``. - ``include_server_fingerprint`` gates *only* the server-key hash. Its sole - consumer is the community usage report, so callers pass ``False`` when - community reporting is disabled to skip an SHA-256 that nothing would read - (the server branch then returns ``("server", None)`` — origin unchanged). The - client-key fingerprint is *always* derived under any active sink: it is - deliberate pre-provisioning for the deferred Mixpanel ``distinct_id`` follow-up - that will consume it, so it is never gated here. + The server-key hash is memoized (see :func:`_server_api_key_fingerprint`) so + the observability hot path never re-hashes the fixed server key. The + fingerprint's sole consumer is the community usage report; the analytics sink + only reads the origin. The raw key is never returned, logged, or used anywhere but as the hash input. @@ -301,11 +315,7 @@ def compute_auth_signals(ctx: Any, include_server_fingerprint: bool = True) -> t # _Absent — fall back to whether a server key is configured. if config.pro_api_key: - # The server-key fingerprint's only consumer is the community usage - # report; skip the hash entirely when that sink is disabled. - if not include_server_fingerprint: - return "server", None - return "server", _fingerprint_pro_api_key(config.pro_api_key) + return "server", _server_api_key_fingerprint(config.pro_api_key) return "none", None diff --git a/blockscout_mcp_server/telemetry.py b/blockscout_mcp_server/telemetry.py index f1912590..2aa023ab 100644 --- a/blockscout_mcp_server/telemetry.py +++ b/blockscout_mcp_server/telemetry.py @@ -31,18 +31,17 @@ def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: signals — analytics is off (not HTTP mode) **and** community telemetry is disabled. In that state both sinks short-circuit on their own gates before reading these values, so skipping derivation changes nothing that is emitted - while avoiding a per-call SHA-256 of the configured server key. The guard is + while avoiding the ``ctx`` extraction and key hashing. The guard is deliberately a *superset* of the precise per-sink gates (it may still derive in a rare config where only the suppressed sink would have run); erring toward deriving keeps this cheap check independent of the sinks' internal logic. When HTTP mode is off the analytics sink early-returns anyway, so a ``None`` origin from this short-circuit never reaches its property bag. - The server-key fingerprint is gated on the community sink: its only consumer - is the community usage report, so ``include_server_fingerprint`` is passed as - ``not config.disable_community_telemetry`` to skip that SHA-256 when community - reporting is off. The client-key fingerprint is unaffected — it is still - derived for the analytics sink and the deferred identity follow-up. + The server-key fingerprint (the fingerprint's only consumer is the community + usage report) is memoized in :func:`compute_auth_signals`, so it costs at most + one SHA-256 per process rather than one per call — there is nothing to gate on + the sinks' precise send conditions. Never raises: :func:`compute_auth_signals` is defensive today, but the guard is kept so this observability concern can never propagate into the tool body even @@ -53,7 +52,7 @@ def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: if not analytics.is_http_mode_enabled() and config.disable_community_telemetry: return None, None try: - return compute_auth_signals(ctx, include_server_fingerprint=not config.disable_community_telemetry) + return compute_auth_signals(ctx) except Exception: return None, None diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index 8b60d40f..cab77006 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -116,56 +116,52 @@ def test_auth_signals_absent_with_no_server_key_returns_none_and_no_fingerprint( # =========================================================================== -# include_server_fingerprint gate — server hash only when a consumer exists +# Server-key fingerprint memoization — the constant server hash is computed once # =========================================================================== # -# The server-key fingerprint's only consumer is the community usage report, so -# resolve_auth_signals passes include_server_fingerprint=False when community -# reporting is disabled. Only the absent-client / server-key branch is gated; -# the client-key path is deliberately never gated (pre-provisioning for the -# deferred distinct_id follow-up). +# The server key is fixed for the process lifetime, so its SHA-256 is memoized +# (see _server_api_key_fingerprint) and the observability hot path reuses the one +# digest instead of re-hashing the same constant on every tool call / resource read. -def test_auth_signals_absent_with_server_key_skips_server_hash_when_gated(monkeypatch): - """Absent client key + server key + include_server_fingerprint=False -> ("server", None). +def test_server_fingerprint_is_memoized_across_calls(monkeypatch): + """Absent client key + server key -> ("server", ), hashing the server key once. - Origin stays "server"; only the SHA-256 is skipped because nothing consumes it. + Pins the optimization that replaces the old ``include_server_fingerprint`` + gate: rather than skipping the hash when no consumer exists, the fixed server + key is hashed once and reused. The spy below proves a second derivation reuses + the memoized digest instead of re-running the SHA-256. """ monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") - assert compute_auth_signals(ctx, include_server_fingerprint=False) == ("server", None) - + pro_api_key_context._server_api_key_fingerprint.cache_clear() -def test_auth_signals_valid_client_hash_not_gated_by_server_flag(monkeypatch): - """A valid client key still yields ("client", ) even with the flag False. + fingerprint_spy = MagicMock(side_effect=pro_api_key_context._fingerprint_pro_api_key) + monkeypatch.setattr(pro_api_key_context, "_fingerprint_pro_api_key", fingerprint_spy) - include_server_fingerprint gates only the server-key branch; the client-key - fingerprint is preserved as pre-provisioning for the deferred identity work. - """ - monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") - assert compute_auth_signals(ctx, include_server_fingerprint=False) == ( - "client", - _expected_fingerprint("client-key-123"), - ) + ctx = _ctx_with_header(_HEADER_NAME, "") # absent client key -> server fallback + expected = ("server", _expected_fingerprint("server-key")) + assert compute_auth_signals(ctx) == expected + assert compute_auth_signals(ctx) == expected + # Two derivations, but the constant server key was hashed exactly once. + fingerprint_spy.assert_called_once_with("server-key") -def test_compute_auth_signals_skips_server_hash_when_gated_off(monkeypatch): - """With ``include_server_fingerprint=False`` the server branch derives the origin - WITHOUT computing the server-key SHA-256. +def test_client_fingerprint_is_never_memoized(monkeypatch): + """A per-request client key is re-hashed each call — only the server key is cached. - Pins the optimization: on the absent-client / configured-server-key branch, - ``_fingerprint_pro_api_key`` must never be called when the flag is False. If a - future change drops the gate, the spy below records a call and this test fails. + The client-key path must never route through the memoizing helper: caching a + per-request secret would both retain it and let a stale digest leak across + requests. Two calls with a valid client header therefore hash it twice. """ monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) - monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) + monkeypatch.setattr(config, "pro_api_key", "", raising=False) fingerprint_spy = MagicMock(side_effect=pro_api_key_context._fingerprint_pro_api_key) monkeypatch.setattr(pro_api_key_context, "_fingerprint_pro_api_key", fingerprint_spy) - ctx = _ctx_with_header(_HEADER_NAME, "") # absent client key -> server fallback - assert compute_auth_signals(ctx, include_server_fingerprint=False) == ("server", None) - fingerprint_spy.assert_not_called() + ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + expected = ("client", _expected_fingerprint("client-key-123")) + assert compute_auth_signals(ctx) == expected + assert compute_auth_signals(ctx) == expected + assert fingerprint_spy.call_count == 2 diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index ec94020c..41191ebb 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -1,4 +1,5 @@ # SPDX-License-Identifier: LicenseRef-Blockscout +import hashlib from types import SimpleNamespace from unittest.mock import ANY, AsyncMock, MagicMock, patch @@ -10,6 +11,7 @@ from blockscout_mcp_server.constants import ( COMMUNITY_TELEMETRY_ENDPOINT, COMMUNITY_TELEMETRY_URL, + PRO_API_KEY_HASH_PREFIX, RESOURCE_READ_EVENT, ) @@ -36,8 +38,8 @@ def test_resolve_auth_signals_short_circuits_when_all_telemetry_disabled(monkeyp def test_resolve_auth_signals_derives_when_community_enabled(monkeypatch): """Community telemetry enabled (HTTP mode off) still needs the signals -> derivation runs. - The community sink consumes the server-key fingerprint, so the server hash is - requested (``include_server_fingerprint=True``). + The community sink consumes the fingerprint, so the helper delegates to + compute_auth_signals and returns its pair unchanged. """ monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) analytics.set_http_mode(False) @@ -46,14 +48,14 @@ def test_resolve_auth_signals_derives_when_community_enabled(monkeypatch): ctx = object() assert telemetry.resolve_auth_signals(ctx) == ("server", "d" * 64) - compute_spy.assert_called_once_with(ctx, include_server_fingerprint=True) + compute_spy.assert_called_once_with(ctx) def test_resolve_auth_signals_derives_in_http_mode_even_if_community_disabled(monkeypatch): """HTTP mode on feeds the analytics sink, so derivation runs even when community is disabled. - Community is off, so the server-key fingerprint has no consumer: the flag is - threaded through as ``include_server_fingerprint=False``. + The short-circuit is a superset guard keyed only on "no sink at all"; with HTTP + mode on it never fires, so the helper delegates to compute_auth_signals. """ monkeypatch.setattr(config, "disable_community_telemetry", True, raising=False) analytics.set_http_mode(True) @@ -63,16 +65,17 @@ def test_resolve_auth_signals_derives_in_http_mode_even_if_community_disabled(mo ctx = object() assert telemetry.resolve_auth_signals(ctx) == ("client", "e" * 64) - compute_spy.assert_called_once_with(ctx, include_server_fingerprint=False) + compute_spy.assert_called_once_with(ctx) finally: analytics.set_http_mode(False) -def test_resolve_auth_signals_skips_server_fingerprint_when_community_disabled(monkeypatch): - """End-to-end gate: absent client key + server key + HTTP on + community off -> ("server", None). +def test_resolve_auth_signals_returns_server_fingerprint_in_http_mode(monkeypatch): + """End-to-end: absent client key + server key + HTTP on + community off -> ("server", ). - Uses the real compute_auth_signals (no spy) to prove the flag is threaded - through and the server-key SHA-256 is skipped, while auth_origin stays "server". + Uses the real compute_auth_signals (no spy). The server-key SHA-256 is no longer + gated away in this exact config — it is memoized once per process — so the helper + returns the full ("server", fingerprint) pair rather than ("server", None). """ monkeypatch.setattr(config, "disable_community_telemetry", True, raising=False) monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) @@ -84,7 +87,8 @@ def test_resolve_auth_signals_skips_server_fingerprint_when_community_disabled(m request=SimpleNamespace(headers=Headers(headers={"Blockscout-MCP-Pro-Api-Key": ""})) ) ) - assert telemetry.resolve_auth_signals(ctx) == ("server", None) + expected = hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}server-key".encode()).hexdigest() + assert telemetry.resolve_auth_signals(ctx) == ("server", expected) finally: analytics.set_http_mode(False) From 344d2a69724915ba0a95686033b48fb5fde9ea54 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 21:02:58 -0600 Subject: [PATCH 17/26] Tolerate unrecognized auth_origin instead of 422-ing the report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Community usage reports arrive fire-and-forget from independently-versioned instances, and a rejection is silent on both ends (the sender ignores the response; the receiver returns 422 with no log). So a strictly-validated auth_origin meant any value this receiver did not recognize — a future enum member rolled out to reporters first, wrong capitalization, or junk — dropped the entire otherwise-valid report with no visibility. Mirror the existing api_key_fingerprint coercion: a mode="before" validator coerces any value outside AuthOrigin (plus null) to None, which track_community_usage already renders as the "unknown" bucket that legacy field-absent reports fall into. The coercion is logged at debug so version skew stays observable. This makes both forward-compat wire signals symmetric and honors issue #423's backward-compatibility goal under version skew. Co-Authored-By: Claude Opus 4.8 --- SPEC.md | 2 +- blockscout_mcp_server/models.py | 36 +++++++++++++++++++++++++++++---- tests/api/test_routes.py | 16 ++++++++++++--- tests/test_tool_usage_report.py | 22 +++++++++++++++----- 4 files changed, 63 insertions(+), 13 deletions(-) diff --git a/SPEC.md b/SPEC.md index 30701692..254212c5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -936,7 +936,7 @@ Intent analytics should favor derived signals over raw text. - **HTTP mode**: Active only when both `BLOCKSCOUT_MIXPANEL_TOKEN` is not configured AND `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` is not set to true - **Mechanism**: To understand usage in the open-source community, these instances send an anonymous, "fire-and-forget" report to a central endpoint (`POST /v1/report_tool_usage`) on the official Blockscout MCP server. This report contains the tool name, tool arguments, the MCP client name and version, the model context protocol version, and the server's version. - **Central Processing**: The central server receives this report, uses the sender's IP address for geolocation, and forwards the event to Mixpanel with the client metadata, protocol version, and a `source` property of `"community"`. This allows us to gather valuable aggregate statistics without requiring every user to have a Mixpanel account. -- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because nothing consumes the fingerprint yet, an invalid one is tolerated rather than allowed to drop an otherwise-valid report — preserving a "present ⇒ valid" invariant the follow-up will rely on — whereas the already-consumed `auth_origin` is validated strictly. Consuming and retaining it for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (The exact field names, enum values, and hex-shape constraints are documented in `API.md`.) +- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because these reports arrive fire-and-forget from independently-versioned community instances (with any rejection silent on both ends), both new fields tolerate unrecognized wire values rather than dropping the whole otherwise-valid report: an invalid fingerprint is coerced to null (preserving a "present ⇒ valid" invariant the follow-up will rely on), and an `auth_origin` this receiver does not recognize — a future enum member, wrong case, or junk — is likewise coerced to null and bucketed as `unknown` downstream (the same bucket legacy field-absent reports fall into), with the coercion logged for version-skew visibility. Consuming and retaining the fingerprint for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (The exact field names, enum values, and hex-shape constraints are documented in `API.md`.) - **Opt-Out**: This community reporting can be completely disabled by setting the `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` environment variable to `true`. ##### Resource-Read Observability diff --git a/blockscout_mcp_server/models.py b/blockscout_mcp_server/models.py index c4143fb2..7371a7a4 100644 --- a/blockscout_mcp_server/models.py +++ b/blockscout_mcp_server/models.py @@ -3,7 +3,7 @@ import logging import re -from typing import Any, Generic, TypeVar +from typing import Any, Generic, TypeVar, get_args from pydantic import BaseModel, ConfigDict, Field, field_validator @@ -15,6 +15,11 @@ # hashlib.sha256(...).hexdigest() digest. _FINGERPRINT_PATTERN = re.compile(r"[0-9a-f]{64}") +# The wire-valid `auth_origin` values, derived from the `AuthOrigin` Literal so the two +# never drift. A value outside this set (plus JSON null) is coerced to None rather than +# rejected — see ``_tolerate_unknown_auth_origin``. +_VALID_AUTH_ORIGINS = frozenset(get_args(AuthOrigin)) + # --- Generic Type Variable --- T = TypeVar("T") @@ -31,7 +36,10 @@ class ToolUsageReport(BaseModel): "The origin of the authorization available to back the reported call: 'client' for a " "client-supplied PRO API key, 'server' for a server-configured key, or 'none' for " "no usable key. Reflects the request's authorization context, not whether the invoked " - "tool actually consumed a key. Absent on legacy payloads that predate this field." + "tool actually consumed a key. Absent on legacy payloads that predate this field. An " + "unrecognized value (unknown enum member, wrong case, or non-string) is coerced to null " + "and reported as 'unknown' downstream, so a version-skewed reporter never drops an " + "otherwise-valid report." ), ) api_key_fingerprint: str | None = Field( @@ -46,6 +54,26 @@ class ToolUsageReport(BaseModel): ), ) + @field_validator("auth_origin", mode="before") + @classmethod + def _tolerate_unknown_auth_origin(cls, value: Any) -> str | None: + """Coerce an unrecognized ``auth_origin`` to ``None`` (→ ``unknown`` downstream) instead of rejecting. + + ``auth_origin`` is consumed as a Mixpanel dimension, but the reports arrive fire-and-forget + from independently-versioned community instances, so a value this receiver does not (yet) + recognize — a future enum member rolled out to reporters first, wrong capitalization, or + non-string junk — must not 422 the whole otherwise-valid report. Anything outside + ``_VALID_AUTH_ORIGINS`` (plus JSON null) is coerced to ``None``, which + :func:`analytics.track_community_usage` already renders as the ``unknown`` bucket that legacy + (field-absent) reports land in. This keeps both forward-compat wire signals symmetric with + ``api_key_fingerprint`` below; the debug log preserves visibility into version skew that a + silent 422 (dropped on both ends) would hide. + """ + if value is None or (isinstance(value, str) and value in _VALID_AUTH_ORIGINS): + return value + logger.debug("Coercing unrecognized auth_origin to None (value=%r)", value) + return None + @field_validator("api_key_fingerprint", mode="before") @classmethod def _tolerate_malformed_fingerprint(cls, value: Any) -> str | None: @@ -55,8 +83,8 @@ def _tolerate_malformed_fingerprint(cls, value: Any) -> str | None: malformed value must not drop an otherwise-valid community report. Any value that is not ``None`` and not a ``str`` matching the 64-lowercase-hex ``_FINGERPRINT_PATTERN`` (including non-string junk) is coerced to ``None``, preserving the "present ⇒ valid 64-hex" invariant - for the deferred identity follow-up. ``auth_origin`` stays strictly validated because it is - consumed. + for the deferred identity follow-up. ``auth_origin`` is coerced the same way (to ``None`` → + ``unknown``) by :meth:`_tolerate_unknown_auth_origin` above. """ if value is None or (isinstance(value, str) and _FINGERPRINT_PATTERN.fullmatch(value)): return value diff --git a/tests/api/test_routes.py b/tests/api/test_routes.py index 0def1aef..c34030b9 100644 --- a/tests/api/test_routes.py +++ b/tests/api/test_routes.py @@ -795,8 +795,15 @@ async def test_report_tool_usage_legacy_payload_without_new_fields(mock_track, c @pytest.mark.asyncio -async def test_report_tool_usage_rejects_bad_auth_origin_enum(client: AsyncClient): - """An out-of-enum auth_origin value is rejected with a 422.""" +@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") +async def test_report_tool_usage_tolerates_unrecognized_auth_origin(mock_track, client: AsyncClient): + """An unrecognized auth_origin does not drop the otherwise-valid report. + + The value this receiver does not recognize is coerced to None by the model (rendered as + "unknown" downstream), so the report is still accepted (202) and forwarded once with + `auth_origin is None`. This keeps a version-skewed community reporter reporting instead of + silently losing 100% of its telemetry to a 422 that the fire-and-forget sender never sees. + """ payload = { "tool_name": "dummy", "tool_args": {"a": 1}, @@ -807,7 +814,10 @@ async def test_report_tool_usage_rejects_bad_auth_origin_enum(client: AsyncClien } headers = {"User-Agent": "BlockscoutMCP/0.0"} response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) - assert response.status_code == 422 + assert response.status_code == 202 + mock_track.assert_called_once() + _, kwargs = mock_track.call_args + assert kwargs["report"].auth_origin is None @pytest.mark.asyncio diff --git a/tests/test_tool_usage_report.py b/tests/test_tool_usage_report.py index bdcba77c..64d1e79a 100644 --- a/tests/test_tool_usage_report.py +++ b/tests/test_tool_usage_report.py @@ -2,7 +2,6 @@ """Tests for the `ToolUsageReport` Pydantic model's optional auth-signal fields.""" import pytest -from pydantic import ValidationError from blockscout_mcp_server.models import ToolUsageReport @@ -37,10 +36,23 @@ def test_auth_origin_round_trips_each_valid_value(origin): assert report.auth_origin == origin -def test_auth_origin_rejects_out_of_enum_value(): - """A payload supplying an out-of-enum auth_origin raises a pydantic ValidationError.""" - with pytest.raises(ValidationError): - ToolUsageReport(**_base_payload(auth_origin="bogus")) +@pytest.mark.parametrize( + "unrecognized", + ["bogus", "Client", "SERVER", "unknown", 123, ["client"], {"origin": "client"}, 4.5], +) +def test_auth_origin_tolerates_unrecognized_value(unrecognized): + """An unrecognized auth_origin is coerced to None rather than rejected. + + Covers out-of-enum ("bogus"), wrong case ("Client"/"SERVER"), the "unknown" Mixpanel sentinel + (never a valid wire value), and non-string junk. auth_origin is consumed, but reports arrive + fire-and-forget from independently-versioned community instances, so a value this receiver does + not recognize must not drop the whole otherwise-valid report; None is rendered as the "unknown" + bucket downstream. The `mode="before"` validator guards with `isinstance(value, str)`, so + non-string input is tolerated too. + """ + report = ToolUsageReport(**_base_payload(auth_origin=unrecognized)) + + assert report.auth_origin is None def test_api_key_fingerprint_round_trips_valid_value(): From f51ee301b8814f0cfb8685a9ea0b631d4db3f9f1 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 21:20:34 -0600 Subject: [PATCH 18/26] Address review: single source of truth for key precedence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute_auth_signals mirrored resolve_pro_api_key's client->server precedence with a second isinstance-over-state chain, coupled only by a docstring. A future precedence change (new key source, different malformed handling) would land in one chain and silently skip the other, so auth_origin would diverge from the key actually enforced — the exact consistency issue #423 exists to protect. Extract _apply_key_precedence(state) as the sole owner of the precedence, returning a discriminated decision (_UseClientKey / _RejectMalformed / _UseServerKey). resolve_pro_api_key now only applies raise/ContextVar semantics to that decision; compute_auth_signals only maps it to an (origin, fingerprint) pair. Neither re-decides precedence. The decision is richer than a bare (origin, effective_key | None): it keeps _RejectMalformed distinct from an absent-with-empty-server key so resolve still emits its two separate errors (malformed vs not-configured). Behavior of both public functions is unchanged. Add a parametrized coupling test asserting the reported auth_origin stays consistent with the key resolve_pro_api_key uses, so a future re-divergence of the two wrappers fails in CI rather than at review. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/pro_api_key_context.py | 125 ++++++++++++++---- .../test_pro_api_key_context_auth_signals.py | 92 ++++++++++++- 2 files changed, 190 insertions(+), 27 deletions(-) diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index dd32bee3..96957a59 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -9,6 +9,9 @@ - A normalization/validation helper. - An extractor that reads the key from any HTTP request context (MCP-over-HTTP or REST). +- A single precedence decision (``_apply_key_precedence``) shared by the resolver + and the analytics signal derivation, so the enforced key and the reported + ``auth_origin`` cannot drift apart. - A resolver that applies the client-key → server-key precedence rule. - A ``require_pro_api_key()`` helper that wraps the resolver with the standard not-configured error so every PRO API entry point raises the same message. @@ -101,6 +104,69 @@ class _Malformed: _MALFORMED: ClientKeyState = _Malformed() +# --------------------------------------------------------------------------- +# 1b. Precedence decision — the single source of truth over ClientKeyState +# --------------------------------------------------------------------------- +# +# The client-key → server-key precedence is decided in exactly one place, +# ``_apply_key_precedence``. Both the enforcement path +# (``resolve_pro_api_key``) and the telemetry path (``compute_auth_signals``) +# consume its result and only *format* it — the former with raise/ContextVar +# semantics, the latter with an ``(origin, fingerprint)`` pair. Because neither +# re-decides precedence, the enforced key and the reported ``auth_origin`` can +# never disagree, and a future precedence change (a new key source, different +# handling of a malformed key) is a one-line edit here that both paths follow +# automatically. This is the consistency issue #423 exists to protect. + + +@dataclass(frozen=True) +class _UseClientKey: + """A valid client key won precedence; ``key`` is the effective PRO API key.""" + + key: str + + +@dataclass(frozen=True) +class _RejectMalformed: + """A malformed client key was supplied — reject it with no fallback to the server key.""" + + +@dataclass(frozen=True) +class _UseServerKey: + """No client key; the configured server key is effective (``key`` is ``""`` when unset).""" + + key: str + + +# The three mutually exclusive precedence outcomes. +_AuthDecision = _UseClientKey | _RejectMalformed | _UseServerKey + + +def _apply_key_precedence(state: ClientKeyState) -> _AuthDecision: + """Map a client-key *state* to the effective auth decision (single source of truth). + + Precedence — this **is** the definition that both :func:`resolve_pro_api_key` + and :func:`compute_auth_signals` consume; it is not mirrored anywhere else: + + 1. Valid client key → :class:`_UseClientKey` (use the client value). + 2. Malformed client key → :class:`_RejectMalformed` (no fallback to the server + key — the request must fail rather than silently downgrade to the server). + 3. No client key (absent) → :class:`_UseServerKey` carrying + ``config.pro_api_key`` (which may be ``""`` when no server key is + configured). + + Pure and total over :data:`ClientKeyState`: it never raises and never hashes. + The raise/ContextVar semantics live in :func:`resolve_pro_api_key`; the + origin/hashing semantics live in :func:`compute_auth_signals`. + """ + if isinstance(state, _Valid): + return _UseClientKey(key=state.value) + if isinstance(state, _Malformed): + return _RejectMalformed() + # _Absent — fall back to the configured server key (may be ""). + return _UseServerKey(key=config.pro_api_key) + + # --------------------------------------------------------------------------- # 2. Module-level ContextVars (not per-call — required for correct semantics) # --------------------------------------------------------------------------- @@ -239,11 +305,13 @@ def extract_client_pro_api_key_from_ctx(ctx: Any) -> ClientKeyState: # # Analytics and telemetry (e.g. log_tool_invocation / community reporting) run # *outside* @pro_api_key_scope, so they cannot read the _client_key_state -# ContextVar (see the @pro_api_key_scope docstring above). These helpers derive -# the same client-key → server-key precedence signal directly from ctx headers -# instead, by reusing extract_client_pro_api_key_from_ctx(). They must NOT call -# resolve_pro_api_key(): it reads the request-scoped ContextVar (unset on this -# path) and raises on a malformed key, whereas these helpers must never raise. +# ContextVar (see the @pro_api_key_scope docstring above). These helpers read the +# key state directly from ctx headers via extract_client_pro_api_key_from_ctx() +# and then apply the *same* precedence through _apply_key_precedence() — only the +# state *source* differs (ctx headers here vs the ContextVar on the enforcement +# path). They must NOT call resolve_pro_api_key(): it reads the request-scoped +# ContextVar (unset on this path) and raises on a malformed key, whereas these +# helpers must never raise. def _fingerprint_pro_api_key(key: str) -> str: @@ -288,14 +356,15 @@ def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: Never raises (delegates to :func:`extract_client_pro_api_key_from_ctx`, which never raises) and never calls :func:`resolve_pro_api_key` (it reads the request-scoped ContextVar — unset on this path — and raises on a malformed - key). The precedence mirrors :func:`resolve_pro_api_key`: + key). Precedence is not re-decided here: it delegates to + :func:`_apply_key_precedence` — the very decision :func:`resolve_pro_api_key` + enforces — and only maps the outcome to an origin + fingerprint: - - Valid client key → ``("client", )``. - - Malformed client key → ``("none", None)`` (no fallback to the server key - for a malformed submission — the request will fail at the PRO API - chokepoint anyway). - - No client key (absent) → ``("server", )`` when - ``config.pro_api_key`` is truthy, otherwise ``("none", None)``. + - :class:`_UseClientKey` → ``("client", )``. + - :class:`_RejectMalformed` → ``("none", None)`` (a malformed submission has + no usable key and will fail at the PRO API chokepoint anyway). + - :class:`_UseServerKey` → ``("server", )`` when a + server key is configured, otherwise ``("none", None)``. The server-key hash is memoized (see :func:`_server_api_key_fingerprint`) so the observability hot path never re-hashes the fixed server key. The @@ -305,17 +374,17 @@ def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: The raw key is never returned, logged, or used anywhere but as the hash input. """ - state = extract_client_pro_api_key_from_ctx(ctx) + decision = _apply_key_precedence(extract_client_pro_api_key_from_ctx(ctx)) - if isinstance(state, _Valid): - return "client", _fingerprint_pro_api_key(state.value) + if isinstance(decision, _UseClientKey): + return "client", _fingerprint_pro_api_key(decision.key) - if isinstance(state, _Malformed): + if isinstance(decision, _RejectMalformed): return "none", None - # _Absent — fall back to whether a server key is configured. - if config.pro_api_key: - return "server", _server_api_key_fingerprint(config.pro_api_key) + # _UseServerKey — "server" only when a key is actually configured. + if decision.key: + return "server", _server_api_key_fingerprint(decision.key) return "none", None @@ -327,7 +396,11 @@ def compute_auth_signals(ctx: Any) -> tuple[AuthOrigin, str | None]: def resolve_pro_api_key() -> str: """Return the effective PRO API key for the current request. - Precedence: + Precedence is decided by :func:`_apply_key_precedence` (shared with + :func:`compute_auth_signals`, so the enforced key and the reported + ``auth_origin`` can never drift apart). This function only applies the + ContextVar/raise semantics to that decision: + 1. Client-supplied key (valid) → use it. 2. Client-supplied key (malformed) → raise ``ValueError`` immediately. No fallback to the server key for a malformed submission. @@ -335,19 +408,19 @@ def resolve_pro_api_key() -> str: Callers' existing emptiness guards handle the ``""`` case. """ - state = _client_key_state.get() + decision = _apply_key_precedence(_client_key_state.get()) - if isinstance(state, _Valid): - return state.value + if isinstance(decision, _UseClientKey): + return decision.key - if isinstance(state, _Malformed): + if isinstance(decision, _RejectMalformed): raise ValueError( "The supplied PRO API key header value is malformed: it contains control characters " "or exceeds the maximum allowed length. Please provide a valid key." ) - # _Absent — fall back to the configured server key. - return config.pro_api_key + # _UseServerKey — the configured server key, possibly "". + return decision.key # --------------------------------------------------------------------------- diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index cab77006..f85ec53b 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -12,12 +12,17 @@ from types import SimpleNamespace from unittest.mock import MagicMock +import pytest from starlette.datastructures import Headers from blockscout_mcp_server import pro_api_key_context from blockscout_mcp_server.config import config from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX -from blockscout_mcp_server.pro_api_key_context import compute_auth_signals +from blockscout_mcp_server.pro_api_key_context import ( + compute_auth_signals, + extract_client_pro_api_key_from_ctx, + resolve_pro_api_key, +) _HEADER_NAME = "Blockscout-MCP-Pro-Api-Key" @@ -165,3 +170,88 @@ def test_client_fingerprint_is_never_memoized(monkeypatch): assert compute_auth_signals(ctx) == expected assert compute_auth_signals(ctx) == expected assert fingerprint_spy.call_count == 2 + + +# =========================================================================== +# Coupling invariant: auth_origin must never disagree with resolve_pro_api_key +# =========================================================================== +# +# Both paths delegate precedence to _apply_key_precedence, so the reported origin +# and the key resolve_pro_api_key() actually uses are two views of one decision. +# This pins the end-to-end invariant directly (not just the shared helper): if a +# future edit re-introduces a divergent branch in either wrapper — the drift risk +# #423 exists to prevent — one of these cases fails. compute_auth_signals is +# driven from ctx headers; resolve_pro_api_key from the ContextVar set to the +# *same* state extracted from that ctx, so both see one logical input. + + +def _resolve_outcome(state: pro_api_key_context.ClientKeyState) -> tuple[str, str | None]: + """Run ``resolve_pro_api_key`` with the ContextVar set to *state*; report its outcome. + + Returns ``("key", )`` for a returned key (possibly ``""``) or + ``("raised", None)`` when the malformed-key ``ValueError`` is raised. + """ + token = pro_api_key_context._client_key_state.set(state) + try: + return "key", resolve_pro_api_key() + except ValueError: + return "raised", None + finally: + pro_api_key_context._client_key_state.reset(token) + + +@pytest.mark.parametrize( + ("server_key", "make_ctx", "expected_origin", "expected_resolution"), + [ + pytest.param( + "server-key", + lambda: _ctx_with_header(_HEADER_NAME, "client-key-123"), + "client", + ("key", "client-key-123"), + id="valid-client-wins", + ), + pytest.param( + "server-key", + lambda: _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey"), + "none", + ("raised", None), + id="malformed-rejected-no-fallback", + ), + pytest.param( + "server-key", + lambda: _ctx_with_header(_HEADER_NAME, ""), + "server", + ("key", "server-key"), + id="absent-falls-back-to-server", + ), + pytest.param( + "", + lambda: _ctx_with_header(_HEADER_NAME, ""), + "none", + ("key", ""), + id="absent-no-server-key", + ), + ], +) +def test_auth_origin_stays_consistent_with_resolve_pro_api_key( + monkeypatch, server_key, make_ctx, expected_origin, expected_resolution +): + monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) + monkeypatch.setattr(config, "pro_api_key", server_key, raising=False) + pro_api_key_context._server_api_key_fingerprint.cache_clear() + + ctx = make_ctx() + origin, _fingerprint = compute_auth_signals(ctx) + assert origin == expected_origin + + # resolve sees the SAME state the telemetry path derived from ctx. + state = extract_client_pro_api_key_from_ctx(ctx) + resolution = _resolve_outcome(state) + assert resolution == expected_resolution + + # The invariant tying the two together: a "usable key" origin iff resolve + # yields a non-empty key; "none" iff resolve raises or yields "". + if origin in ("client", "server"): + assert resolution[0] == "key" and resolution[1] + else: + assert resolution == ("raised", None) or resolution == ("key", "") From edc009c7e13fa6d9e150d23bc9b6d8e697a93971 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 21:45:48 -0600 Subject: [PATCH 19/26] Address review: trim SPEC/AGENTS docs per documentation rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC.md: the Authorization-context bullet restated the auth_origin/ fingerprint validation mechanics nearly verbatim from the ToolUsageReport validator docstrings and API.md, violating rule 1 (keep SPEC.md lean; details discoverable through code). Trim to the decision level — new dimension + fingerprint, privacy posture, and the version-skew rationale for tolerating unknown wire values — dropping the coercion mechanics (null coercion, invalid-value taxonomy, present-implies-valid invariant, downstream bucketing, skew logging). Point to API.md for the wire shape and to the validators for the coercion behavior. AGENTS.md: drop the 'runs outside @pro_api_key_scope' implementation detail from the compute_auth_signals() entry per rule 2 (search hint, not how-it-works prose); keep the brief consumer note at the altitude of neighboring entries. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 2 +- SPEC.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cd0aba55..a7a42d9e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -398,7 +398,7 @@ mcp-server/ * **`pro_api_key_context.py`**: * Owns request-scoped resolution of a client-supplied Blockscout PRO API key, kept separate from logging/observability. * Provides a `ContextVar` of the per-request client-key state, a normalization/validation helper, `extract_client_pro_api_key_from_ctx()`, `resolve_pro_api_key()` (precedence: valid client key → server key → not-configured error; malformed client key → terminal error, no fallback), and the `@pro_api_key_scope` decorator. - * Also provides the `ctx`-derived helper `compute_auth_signals()` (used by the analytics and community-telemetry paths), which runs outside `@pro_api_key_scope`. + * Also provides the `ctx`-derived helper `compute_auth_signals()`, used by the analytics and community-telemetry paths. * Honored for any HTTP request that carries the configured header (MCP-over-HTTP or REST); the key is never logged or placed in cache keys. * Also defines the per-invocation credit-tracking symbols: `CreditSink`, the `_credit_sink` `ContextVar`, and the `@pro_api_credit_scope` decorator (a sibling of `@pro_api_key_scope`). * **`cache.py`**: diff --git a/SPEC.md b/SPEC.md index 254212c5..b994e1dc 100644 --- a/SPEC.md +++ b/SPEC.md @@ -936,7 +936,7 @@ Intent analytics should favor derived signals over raw text. - **HTTP mode**: Active only when both `BLOCKSCOUT_MIXPANEL_TOKEN` is not configured AND `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` is not set to true - **Mechanism**: To understand usage in the open-source community, these instances send an anonymous, "fire-and-forget" report to a central endpoint (`POST /v1/report_tool_usage`) on the official Blockscout MCP server. This report contains the tool name, tool arguments, the MCP client name and version, the model context protocol version, and the server's version. - **Central Processing**: The central server receives this report, uses the sender's IP address for geolocation, and forwards the event to Mixpanel with the client metadata, protocol version, and a `source` property of `"community"`. This allows us to gather valuable aggregate statistics without requiring every user to have a Mixpanel account. -- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because these reports arrive fire-and-forget from independently-versioned community instances (with any rejection silent on both ends), both new fields tolerate unrecognized wire values rather than dropping the whole otherwise-valid report: an invalid fingerprint is coerced to null (preserving a "present ⇒ valid" invariant the follow-up will rely on), and an `auth_origin` this receiver does not recognize — a future enum member, wrong case, or junk — is likewise coerced to null and bucketed as `unknown` downstream (the same bucket legacy field-absent reports fall into), with the coercion logged for version-skew visibility. Consuming and retaining the fingerprint for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (The exact field names, enum values, and hex-shape constraints are documented in `API.md`.) +- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because these reports arrive fire-and-forget from independently-versioned community instances, both new fields tolerate unrecognized wire values rather than dropping an otherwise-valid report. Consuming and retaining the fingerprint for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (Exact field names, enum values, and hex-shape constraints are documented in `API.md`; the coercion behavior lives in the `ToolUsageReport` validators.) - **Opt-Out**: This community reporting can be completely disabled by setting the `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` environment variable to `true`. ##### Resource-Read Observability From 952dc9aa2fc5eff6ff6126ad0d22b7155a2d820f Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 22:07:51 -0600 Subject: [PATCH 20/26] test: dedupe PRO API key ctx builders and fingerprint assertions Address review: extract the duplicated MCP-context builders into a shared tests/pro_api_key_helpers.py, and have the boundary tests (telemetry, observability, decorators) assert the forwarded fingerprint against the production _fingerprint_pro_api_key helper instead of hand-re-deriving prefix+sha256. A single independent scheme re-derivation (_expected_fingerprint) stays in the auth-signals unit tests, so a future scheme change (v1 -> HMAC pepper) touches exactly one place while wiring tests still catch a wrong forwarded hash. Also list the new shared helper in the AGENTS.md project-structure tree. Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 1 + tests/pro_api_key_helpers.py | 45 ++++++++++ tests/test_observability.py | 13 ++- tests/test_pro_api_key_context.py | 84 +++++-------------- .../test_pro_api_key_context_auth_signals.py | 49 +++-------- tests/test_telemetry.py | 15 +--- tests/tools/test_decorators.py | 5 +- 7 files changed, 91 insertions(+), 121 deletions(-) create mode 100644 tests/pro_api_key_helpers.py diff --git a/AGENTS.md b/AGENTS.md index a7a42d9e..22124c47 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -115,6 +115,7 @@ mcp-server/ │ │ ├── test_routes.py # Unit tests for API route definitions │ │ └── test_skill_resource_routes.py # Unit tests for the bundled skill HTTP mirror │ ├── conftest.py +│ ├── pro_api_key_helpers.py # Shared request-context builders for PRO API key / auth-signal tests │ ├── evals/ # Evaluation artifacts and runner configs for tool output checks │ │ ├── .env.example │ │ ├── .gemini/ diff --git a/tests/pro_api_key_helpers.py b/tests/pro_api_key_helpers.py new file mode 100644 index 00000000..ec517fa3 --- /dev/null +++ b/tests/pro_api_key_helpers.py @@ -0,0 +1,45 @@ +# SPDX-License-Identifier: LicenseRef-Blockscout +"""Shared builders for MCP-like request contexts used across PRO API key tests. + +Several test modules independently reconstructed the same request-context shape +to exercise client PRO API key extraction. Centralizing the two flavors here +keeps them in one place and makes the header-encoding contract explicit: + +- :func:`ctx_with_header` uses real :class:`starlette.datastructures.Headers` + and passes the header name in non-canonical (upper) casing so the + case-insensitive lookup path is exercised. Use it for well-formed values. +- :func:`ctx_with_malformed_header` uses a plain ``dict`` so a value that real + starlette ``Headers`` would refuse to latin-1 encode (control characters, + over-length) can still be injected; extraction only needs a case-insensitive + ``Mapping`` lookup. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +from starlette.datastructures import Headers + + +def ctx_with_header(header_name: str, header_value: str) -> SimpleNamespace: + """Build a minimal MCP-like context carrying *header_value* under *header_name*. + + Real :class:`starlette.datastructures.Headers` are used with the header name + upper-cased so the case-insensitive lookup path is exercised. + """ + headers = Headers(headers={header_name.upper(): header_value}) + request = SimpleNamespace(headers=headers) + return SimpleNamespace(request_context=SimpleNamespace(request=request)) + + +def ctx_with_malformed_header(header_name: str, header_value: str) -> SimpleNamespace: + """Build a context whose header value bypasses starlette's encoding checks. + + A plain ``dict`` is used so a value real :class:`starlette.datastructures.Headers` + would refuse to encode (control characters, over-length) can still be + injected. The header name keeps its given casing; extraction is + case-insensitive regardless. + """ + headers = {header_name: header_value} + request = SimpleNamespace(headers=headers) + return SimpleNamespace(request_context=SimpleNamespace(request=request)) diff --git a/tests/test_observability.py b/tests/test_observability.py index 1fb39274..eb8642d6 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -3,12 +3,12 @@ from __future__ import annotations -import hashlib import logging from unittest.mock import MagicMock, patch from pydantic import AnyUrl +from blockscout_mcp_server import pro_api_key_context from blockscout_mcp_server.client_meta import ( UNDEFINED_CLIENT_NAME, UNDEFINED_CLIENT_VERSION, @@ -16,7 +16,6 @@ ClientMeta, format_client_meta_suffix, ) -from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX from blockscout_mcp_server.observability import log_resource_read _SKILL_URI = "blockscout-mcp://skill/SKILL.md" @@ -148,11 +147,11 @@ def test_community_sink_forwards_auth_origin_and_fingerprint(monkeypatch): mock_send_report.assert_called_once() call_kwargs = mock_send_report.call_args.kwargs assert call_kwargs["auth_origin"] == "server" - # Pin the exact server-key fingerprint at this forwarding boundary (not merely "not None"), - # so a regression that forwards a different non-None hash here — e.g. an unprefixed digest or - # the client-key hash — is caught. The value is otherwise only asserted in the helper's own - # unit tests, never at this sink's boundary. - expected_fingerprint = hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}server-key".encode()).hexdigest() + # Pin the forwarded fingerprint against the production helper (not merely "not None"), so a + # regression that forwards a different hash here — an unprefixed digest or the client-key hash — + # is caught. This boundary asserts wiring; the fingerprint *scheme* is re-derived independently + # only in the helper's own unit tests (_expected_fingerprint), so a scheme change touches one place. + expected_fingerprint = pro_api_key_context._fingerprint_pro_api_key("server-key") assert call_kwargs["api_key_fingerprint"] == expected_fingerprint diff --git a/tests/test_pro_api_key_context.py b/tests/test_pro_api_key_context.py index 089ce26a..83118e11 100644 --- a/tests/test_pro_api_key_context.py +++ b/tests/test_pro_api_key_context.py @@ -9,7 +9,7 @@ from unittest.mock import MagicMock import pytest -from starlette.datastructures import Headers +from pro_api_key_helpers import ctx_with_header, ctx_with_malformed_header from blockscout_mcp_server.config import config from blockscout_mcp_server.pro_api_key_context import ( @@ -108,23 +108,10 @@ def test_normalize_exactly_max_length_is_valid(): # =========================================================================== -def _mcp_ctx_with_header(header_name: str, header_value: str) -> SimpleNamespace: - """Build a minimal MCP-like context carrying *header_value* under *header_name*. - - Uses real ``starlette.datastructures.Headers`` with non-canonical casing to - exercise case-insensitive lookup. - """ - # Use non-canonical casing to prove case-insensitive lookup works. - # starlette.datastructures.Headers accepts a Mapping (dict) for the headers param. - headers = Headers(headers={header_name.upper(): header_value}) - request = SimpleNamespace(headers=headers) - return SimpleNamespace(request_context=SimpleNamespace(request=request)) - - def test_extraction_rest_call_source_reads_header(monkeypatch): """A REST-source context that carries the configured header must yield _Valid.""" monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) - ctx = _mcp_ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-123") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-123") ctx.call_source = "rest" # type: ignore[attr-defined] state = extract_client_pro_api_key_from_ctx(ctx) assert isinstance(state, _Valid) @@ -134,7 +121,7 @@ def test_extraction_rest_call_source_reads_header(monkeypatch): def test_extraction_rest_call_source_absent_header_is_absent(monkeypatch): """A REST-source context with no header value yields _Absent.""" monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) - ctx = _mcp_ctx_with_header("Blockscout-MCP-Pro-Api-Key", "") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "") ctx.call_source = "rest" # type: ignore[attr-defined] assert isinstance(extract_client_pro_api_key_from_ctx(ctx), _Absent) @@ -142,34 +129,30 @@ def test_extraction_rest_call_source_absent_header_is_absent(monkeypatch): def test_extraction_rest_call_source_malformed_header_is_malformed(monkeypatch): """A REST-source context with a control-char header value yields _Malformed.""" monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) - # Use a plain dict so we can inject a control-character value that starlette - # would refuse to encode. - headers = {"Blockscout-MCP-Pro-Api-Key": "bad\nkey"} - request = SimpleNamespace(headers=headers) - ctx = SimpleNamespace(request_context=SimpleNamespace(request=request), call_source="rest") + ctx = ctx_with_malformed_header("Blockscout-MCP-Pro-Api-Key", "bad\nkey") + ctx.call_source = "rest" assert isinstance(extract_client_pro_api_key_from_ctx(ctx), _Malformed) def test_extraction_rest_call_source_over_length_header_is_malformed(monkeypatch): """A REST-source context with an over-length header value yields _Malformed.""" monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) - headers = {"Blockscout-MCP-Pro-Api-Key": "a" * 257} - request = SimpleNamespace(headers=headers) - ctx = SimpleNamespace(request_context=SimpleNamespace(request=request), call_source="rest") + ctx = ctx_with_malformed_header("Blockscout-MCP-Pro-Api-Key", "a" * 257) + ctx.call_source = "rest" assert isinstance(extract_client_pro_api_key_from_ctx(ctx), _Malformed) def test_extraction_rest_call_source_disabled_feature_is_absent(monkeypatch): """Feature disabled (empty header config) → absent even if the header is present.""" monkeypatch.setattr(config, "pro_api_key_header", "", raising=False) - ctx = _mcp_ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-123") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-123") ctx.call_source = "rest" # type: ignore[attr-defined] assert isinstance(extract_client_pro_api_key_from_ctx(ctx), _Absent) def test_extraction_empty_header_config_is_absent(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", "", raising=False) - ctx = _mcp_ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-123") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-123") assert isinstance(extract_client_pro_api_key_from_ctx(ctx), _Absent) @@ -194,11 +177,8 @@ def test_extraction_stdio_like_no_request_is_absent(monkeypatch): def test_extraction_mcp_ctx_with_valid_header(monkeypatch): """Real starlette Headers + non-canonical casing → valid state.""" monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) - # Pass the header under ALL-CAPS to prove case-insensitive lookup. - # starlette.datastructures.Headers accepts a Mapping (dict) for the headers param. - headers = Headers(headers={"BLOCKSCOUT-MCP-PRO-API-KEY": "my-client-key"}) - request = SimpleNamespace(headers=headers) - ctx = SimpleNamespace(request_context=SimpleNamespace(request=request)) + # ctx_with_header upper-cases the header name, so this exercises case-insensitive lookup. + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "my-client-key") state = extract_client_pro_api_key_from_ctx(ctx) assert isinstance(state, _Valid) @@ -271,10 +251,7 @@ async def test_malformed_error_does_not_embed_control_char_value(monkeypatch): async def dummy(ctx) -> None: resolve_pro_api_key() - # Plain dict headers: real starlette Headers reject control-char values at - # construction time, while the extractor works with any Mapping. - request = SimpleNamespace(headers={"Blockscout-MCP-Pro-Api-Key": raw_value}) - ctx = SimpleNamespace(request_context=SimpleNamespace(request=request)) + ctx = ctx_with_malformed_header("Blockscout-MCP-Pro-Api-Key", raw_value) with pytest.raises(ValueError) as exc_info: await dummy(ctx=ctx) @@ -295,8 +272,7 @@ async def test_malformed_error_does_not_embed_over_length_value(monkeypatch): async def dummy(ctx) -> None: resolve_pro_api_key() - request = SimpleNamespace(headers={"Blockscout-MCP-Pro-Api-Key": raw_value}) - ctx = SimpleNamespace(request_context=SimpleNamespace(request=request)) + ctx = ctx_with_malformed_header("Blockscout-MCP-Pro-Api-Key", raw_value) with pytest.raises(ValueError) as exc_info: await dummy(ctx=ctx) @@ -308,16 +284,6 @@ async def dummy(ctx) -> None: # =========================================================================== -def _ctx_with_starlette_headers(header_name: str, header_value: str) -> SimpleNamespace: - """Build an MCP-like context using real starlette.datastructures.Headers (dict form). - - The header name is passed in UPPER CASE so the case-insensitive lookup is exercised. - """ - headers = Headers(headers={header_name.upper(): header_value}) - request = SimpleNamespace(headers=headers) - return SimpleNamespace(request_context=SimpleNamespace(request=request)) - - @pytest.mark.asyncio async def test_decorator_sets_valid_state_during_call(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", "Blockscout-MCP-Pro-Api-Key", raising=False) @@ -328,7 +294,7 @@ async def test_decorator_sets_valid_state_during_call(monkeypatch): async def dummy(ctx) -> None: observed_state.append(_client_key_state.get()) - ctx = _ctx_with_starlette_headers("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") await dummy(ctx=ctx) @@ -345,7 +311,7 @@ async def test_decorator_resets_state_after_call(monkeypatch): async def dummy(ctx) -> None: pass - ctx = _ctx_with_starlette_headers("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") await dummy(ctx=ctx) @@ -361,7 +327,7 @@ async def test_decorator_resets_state_even_when_wrapped_function_raises(monkeypa async def dummy(ctx) -> None: raise RuntimeError("boom") - ctx = _ctx_with_starlette_headers("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") with pytest.raises(RuntimeError, match="boom"): await dummy(ctx=ctx) @@ -381,7 +347,7 @@ async def test_decorator_rest_call_source_sets_valid_state(monkeypatch): async def dummy(ctx) -> None: observed_state.append(_client_key_state.get()) - ctx = _ctx_with_starlette_headers("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "client-key-xyz") ctx.call_source = "rest" # type: ignore[attr-defined] await dummy(ctx=ctx) @@ -407,12 +373,7 @@ async def test_decorator_malformed_does_not_raise_from_decorator(monkeypatch): async def dummy(ctx) -> None: ran.append(True) - # Use a plain dict so we can inject a control-character value that starlette - # would refuse to encode — the extractor just calls get_header_case_insensitive - # which works with any Mapping. - headers = {"Blockscout-MCP-Pro-Api-Key": "bad\x00key"} - request = SimpleNamespace(headers=headers) - ctx = SimpleNamespace(request_context=SimpleNamespace(request=request)) + ctx = ctx_with_malformed_header("Blockscout-MCP-Pro-Api-Key", "bad\x00key") # Must not raise at decoration time or call time (malformed raise is in resolve_pro_api_key) await dummy(ctx=ctx) @@ -449,13 +410,8 @@ async def dummy(ctx, task_name: str) -> None: await asyncio.sleep(0) results[task_name] = resolve_pro_api_key() - def _ctx_with_key(key: str) -> SimpleNamespace: - headers = Headers(headers={"Blockscout-MCP-Pro-Api-Key": key}) - request = SimpleNamespace(headers=headers) - return SimpleNamespace(request_context=SimpleNamespace(request=request)) - - ctx_a = _ctx_with_key("key-for-task-a") - ctx_b = _ctx_with_key("key-for-task-b") + ctx_a = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "key-for-task-a") + ctx_b = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "key-for-task-b") await asyncio.gather( dummy(ctx=ctx_a, task_name="a"), diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index f85ec53b..13676510 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -9,11 +9,10 @@ from __future__ import annotations import hashlib -from types import SimpleNamespace from unittest.mock import MagicMock import pytest -from starlette.datastructures import Headers +from pro_api_key_helpers import ctx_with_header, ctx_with_malformed_header from blockscout_mcp_server import pro_api_key_context from blockscout_mcp_server.config import config @@ -27,28 +26,6 @@ _HEADER_NAME = "Blockscout-MCP-Pro-Api-Key" -def _ctx_with_header(header_name: str, header_value: str) -> SimpleNamespace: - """Build a minimal MCP-like context carrying *header_value* under *header_name*. - - Mirrors the helper in ``tests/test_pro_api_key_context.py``. - """ - headers = Headers(headers={header_name.upper(): header_value}) - request = SimpleNamespace(headers=headers) - return SimpleNamespace(request_context=SimpleNamespace(request=request)) - - -def _ctx_with_malformed_header(header_name: str, header_value: str) -> SimpleNamespace: - """Build a context whose header value contains control characters. - - Uses a plain dict so a value that real ``starlette.datastructures.Headers`` - would refuse to encode can still be injected (mirrors the equivalent helper - usage in ``tests/test_pro_api_key_context.py``). - """ - headers = {header_name: header_value} - request = SimpleNamespace(headers=headers) - return SimpleNamespace(request_context=SimpleNamespace(request=request)) - - def _expected_fingerprint(key: str) -> str: """Compute the expected fingerprint via the same explicit UTF-8 byte construction.""" return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 @@ -63,7 +40,7 @@ def test_fingerprint_never_equals_or_contains_raw_key(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "", raising=False) raw_key = "super-secret-client-key-456" - ctx = _ctx_with_header(_HEADER_NAME, raw_key) + ctx = ctx_with_header(_HEADER_NAME, raw_key) fingerprint = compute_auth_signals(ctx)[1] assert fingerprint is not None assert fingerprint != raw_key @@ -75,7 +52,7 @@ def test_fingerprint_prefix_actually_participates(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "", raising=False) raw_key = "client-key-789" - ctx = _ctx_with_header(_HEADER_NAME, raw_key) + ctx = ctx_with_header(_HEADER_NAME, raw_key) fingerprint = compute_auth_signals(ctx)[1] unprefixed_hash = hashlib.sha256(raw_key.encode("utf-8")).hexdigest() assert fingerprint != unprefixed_hash @@ -95,28 +72,28 @@ def test_fingerprint_prefix_actually_participates(monkeypatch): def test_auth_signals_valid_client_returns_client_and_client_hash(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + ctx = ctx_with_header(_HEADER_NAME, "client-key-123") assert compute_auth_signals(ctx) == ("client", _expected_fingerprint("client-key-123")) def test_auth_signals_malformed_returns_none_and_no_fingerprint(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") + ctx = ctx_with_malformed_header(_HEADER_NAME, "bad\nkey") assert compute_auth_signals(ctx) == ("none", None) def test_auth_signals_absent_with_server_key_returns_server_and_server_hash(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") + ctx = ctx_with_header(_HEADER_NAME, "") assert compute_auth_signals(ctx) == ("server", _expected_fingerprint("server-key")) def test_auth_signals_absent_with_no_server_key_returns_none_and_no_fingerprint(monkeypatch): monkeypatch.setattr(config, "pro_api_key_header", _HEADER_NAME, raising=False) monkeypatch.setattr(config, "pro_api_key", "", raising=False) - ctx = _ctx_with_header(_HEADER_NAME, "") + ctx = ctx_with_header(_HEADER_NAME, "") assert compute_auth_signals(ctx) == ("none", None) @@ -144,7 +121,7 @@ def test_server_fingerprint_is_memoized_across_calls(monkeypatch): fingerprint_spy = MagicMock(side_effect=pro_api_key_context._fingerprint_pro_api_key) monkeypatch.setattr(pro_api_key_context, "_fingerprint_pro_api_key", fingerprint_spy) - ctx = _ctx_with_header(_HEADER_NAME, "") # absent client key -> server fallback + ctx = ctx_with_header(_HEADER_NAME, "") # absent client key -> server fallback expected = ("server", _expected_fingerprint("server-key")) assert compute_auth_signals(ctx) == expected assert compute_auth_signals(ctx) == expected @@ -165,7 +142,7 @@ def test_client_fingerprint_is_never_memoized(monkeypatch): fingerprint_spy = MagicMock(side_effect=pro_api_key_context._fingerprint_pro_api_key) monkeypatch.setattr(pro_api_key_context, "_fingerprint_pro_api_key", fingerprint_spy) - ctx = _ctx_with_header(_HEADER_NAME, "client-key-123") + ctx = ctx_with_header(_HEADER_NAME, "client-key-123") expected = ("client", _expected_fingerprint("client-key-123")) assert compute_auth_signals(ctx) == expected assert compute_auth_signals(ctx) == expected @@ -205,28 +182,28 @@ def _resolve_outcome(state: pro_api_key_context.ClientKeyState) -> tuple[str, st [ pytest.param( "server-key", - lambda: _ctx_with_header(_HEADER_NAME, "client-key-123"), + lambda: ctx_with_header(_HEADER_NAME, "client-key-123"), "client", ("key", "client-key-123"), id="valid-client-wins", ), pytest.param( "server-key", - lambda: _ctx_with_malformed_header(_HEADER_NAME, "bad\nkey"), + lambda: ctx_with_malformed_header(_HEADER_NAME, "bad\nkey"), "none", ("raised", None), id="malformed-rejected-no-fallback", ), pytest.param( "server-key", - lambda: _ctx_with_header(_HEADER_NAME, ""), + lambda: ctx_with_header(_HEADER_NAME, ""), "server", ("key", "server-key"), id="absent-falls-back-to-server", ), pytest.param( "", - lambda: _ctx_with_header(_HEADER_NAME, ""), + lambda: ctx_with_header(_HEADER_NAME, ""), "none", ("key", ""), id="absent-no-server-key", diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 41191ebb..82ded5f4 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -1,17 +1,14 @@ # SPDX-License-Identifier: LicenseRef-Blockscout -import hashlib -from types import SimpleNamespace from unittest.mock import ANY, AsyncMock, MagicMock, patch import pytest -from starlette.datastructures import Headers +from pro_api_key_helpers import ctx_with_header -from blockscout_mcp_server import analytics, telemetry +from blockscout_mcp_server import analytics, pro_api_key_context, telemetry from blockscout_mcp_server.config import config from blockscout_mcp_server.constants import ( COMMUNITY_TELEMETRY_ENDPOINT, COMMUNITY_TELEMETRY_URL, - PRO_API_KEY_HASH_PREFIX, RESOURCE_READ_EVENT, ) @@ -82,12 +79,8 @@ def test_resolve_auth_signals_returns_server_fingerprint_in_http_mode(monkeypatc monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False) analytics.set_http_mode(True) try: - ctx = SimpleNamespace( - request_context=SimpleNamespace( - request=SimpleNamespace(headers=Headers(headers={"Blockscout-MCP-Pro-Api-Key": ""})) - ) - ) - expected = hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}server-key".encode()).hexdigest() + ctx = ctx_with_header("Blockscout-MCP-Pro-Api-Key", "") + expected = pro_api_key_context._fingerprint_pro_api_key("server-key") assert telemetry.resolve_auth_signals(ctx) == ("server", expected) finally: analytics.set_http_mode(False) diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index 7ca4cafe..d86ab5f8 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -1,6 +1,5 @@ # SPDX-License-Identifier: LicenseRef-Blockscout import asyncio -import hashlib import logging from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch @@ -18,8 +17,8 @@ UNKNOWN_PROTOCOL_VERSION, ) from blockscout_mcp_server.config import config as server_config -from blockscout_mcp_server.constants import PRO_API_KEY_HASH_PREFIX from blockscout_mcp_server.pro_api_key_context import ( + _fingerprint_pro_api_key, pro_api_key_scope, resolve_pro_api_key, ) @@ -262,7 +261,7 @@ async def dummy_tool(a: int, ctx: Context) -> int: assert "api_key_fingerprint" not in props # ...and the identical pair reached the community report. - expected_fingerprint = hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{raw_key}".encode()).hexdigest() + expected_fingerprint = _fingerprint_pro_api_key(raw_key) mock_report.assert_awaited_once() call_kwargs = mock_report.await_args.kwargs assert call_kwargs["auth_origin"] == "client" From b81df62103b26575b9a5b221e544ee3a80fe604c Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 22:14:12 -0600 Subject: [PATCH 21/26] Address review: drop redundant .encode("utf-8") and UP012 noqas .encode() defaults to UTF-8, so the explicit argument was byte-identical and only forced a UP012 suppression that obscured intent. Use .encode() and remove both noqas. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/pro_api_key_context.py | 2 +- tests/test_pro_api_key_context_auth_signals.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/blockscout_mcp_server/pro_api_key_context.py b/blockscout_mcp_server/pro_api_key_context.py index 96957a59..e67278a4 100644 --- a/blockscout_mcp_server/pro_api_key_context.py +++ b/blockscout_mcp_server/pro_api_key_context.py @@ -323,7 +323,7 @@ def _fingerprint_pro_api_key(key: str) -> str: The raw key is never returned, logged, or placed anywhere but this hash input. """ - return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 + return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode()).hexdigest() @functools.lru_cache(maxsize=1) diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index 13676510..c064b243 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -27,8 +27,8 @@ def _expected_fingerprint(key: str) -> str: - """Compute the expected fingerprint via the same explicit UTF-8 byte construction.""" - return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode("utf-8")).hexdigest() # noqa: UP012 + """Compute the expected fingerprint via the same UTF-8 byte construction.""" + return hashlib.sha256(f"{PRO_API_KEY_HASH_PREFIX}{key}".encode()).hexdigest() # =========================================================================== From 2f436cfb0adfc7a656c517c69176328bc18b58b0 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Wed, 1 Jul 2026 22:46:21 -0600 Subject: [PATCH 22/26] Avoid logging swapped-in secrets in auth_origin coercion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `_tolerate_unknown_auth_origin` validator logged the rejected value verbatim (`value=%r`), while its sibling `_tolerate_malformed_fingerprint` deliberately logs only the type — because a field-swap bug in an independently-versioned reporter could land a raw key or fingerprint in `auth_origin`, and this codebase never logs the key. Log the value verbatim only when it is a short string (legitimate version-skew values are short enum members); anything longer — a 64-hex fingerprint, a raw key, or other junk — is logged by type/length only. Preserves version-skew visibility without leaking secret-shaped content into the central server's debug logs. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/models.py | 25 +++++++++++++++--- tests/test_tool_usage_report.py | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/blockscout_mcp_server/models.py b/blockscout_mcp_server/models.py index 7371a7a4..725203cc 100644 --- a/blockscout_mcp_server/models.py +++ b/blockscout_mcp_server/models.py @@ -20,6 +20,14 @@ # rejected — see ``_tolerate_unknown_auth_origin``. _VALID_AUTH_ORIGINS = frozenset(get_args(AuthOrigin)) +# When an unrecognized `auth_origin` is coerced away, its value is logged verbatim only if it is a +# short string. Legitimate version-skew values are short enum members (the longest valid origin is +# 6 chars), so a short value is both safe and diagnostically useful to log; anything longer — a +# 64-hex fingerprint, a raw key, or other junk swapped into the field by a buggy reporter — is +# logged by type/length only, mirroring ``_tolerate_malformed_fingerprint``'s no-value-in-logs +# posture. The cap sits well above any plausible enum member yet far below a 64-char fingerprint. +_MAX_LOGGABLE_AUTH_ORIGIN_LEN = 16 + # --- Generic Type Variable --- T = TypeVar("T") @@ -66,12 +74,23 @@ def _tolerate_unknown_auth_origin(cls, value: Any) -> str | None: ``_VALID_AUTH_ORIGINS`` (plus JSON null) is coerced to ``None``, which :func:`analytics.track_community_usage` already renders as the ``unknown`` bucket that legacy (field-absent) reports land in. This keeps both forward-compat wire signals symmetric with - ``api_key_fingerprint`` below; the debug log preserves visibility into version skew that a - silent 422 (dropped on both ends) would hide. + ``api_key_fingerprint`` below. The debug log preserves visibility into version skew that a + silent 422 (dropped on both ends) would hide, but — because the report is untrusted, + independently-versioned wire input — it emits the raw value only when it is a short string + (``_MAX_LOGGABLE_AUTH_ORIGIN_LEN``); a longer value (a fingerprint or key swapped into the + field by a buggy reporter) is logged by type and length only, never verbatim, matching the + no-secret-in-logs posture of :meth:`_tolerate_malformed_fingerprint`. """ if value is None or (isinstance(value, str) and value in _VALID_AUTH_ORIGINS): return value - logger.debug("Coercing unrecognized auth_origin to None (value=%r)", value) + if isinstance(value, str) and len(value) <= _MAX_LOGGABLE_AUTH_ORIGIN_LEN: + logger.debug("Coercing unrecognized auth_origin to None (value=%r)", value) + else: + logger.debug( + "Coercing unrecognized auth_origin to None (type=%s, len=%s)", + type(value).__name__, + len(value) if isinstance(value, str) else "n/a", + ) return None @field_validator("api_key_fingerprint", mode="before") diff --git a/tests/test_tool_usage_report.py b/tests/test_tool_usage_report.py index 64d1e79a..68ece9d0 100644 --- a/tests/test_tool_usage_report.py +++ b/tests/test_tool_usage_report.py @@ -1,10 +1,14 @@ # SPDX-License-Identifier: LicenseRef-Blockscout """Tests for the `ToolUsageReport` Pydantic model's optional auth-signal fields.""" +import logging + import pytest from blockscout_mcp_server.models import ToolUsageReport +_MODELS_LOGGER = "blockscout_mcp_server.models" + VALID_FINGERPRINT = "a" * 64 @@ -55,6 +59,48 @@ def test_auth_origin_tolerates_unrecognized_value(unrecognized): assert report.auth_origin is None +def test_short_unrecognized_auth_origin_logs_value(caplog): + """A short unrecognized auth_origin is logged verbatim, preserving version-skew visibility. + + Legitimate skew values are short enum members (e.g. a future "proxy"), so logging the actual + value is both safe and the whole point of the debug line — it lets an operator see *which* + unexpected value a newer reporter sent. + """ + with caplog.at_level(logging.DEBUG, logger=_MODELS_LOGGER): + ToolUsageReport(**_base_payload(auth_origin="proxy")) + + assert "proxy" in caplog.text + + +def test_secret_shaped_auth_origin_is_never_logged_verbatim(caplog): + """A long, secret-shaped unrecognized auth_origin is logged by type+len only, never verbatim. + + Guards the credential-safety invariant: if a buggy reporter swaps a 64-hex fingerprint (or a + raw key) into auth_origin, the content must not leak into the central server's debug logs. This + mirrors the no-value-in-logs posture of the api_key_fingerprint validator. + """ + secret_shaped = "a" * 64 + with caplog.at_level(logging.DEBUG, logger=_MODELS_LOGGER): + ToolUsageReport(**_base_payload(auth_origin=secret_shaped)) + + assert secret_shaped not in caplog.text + assert "type=str" in caplog.text + assert "len=64" in caplog.text + + +def test_non_string_auth_origin_logs_type_without_length(caplog): + """A non-string unrecognized auth_origin logs its type with ``len=n/a`` and never raises. + + The ``len(...)`` diagnostic is guarded by ``isinstance(value, str)``, so junk types (here a + list, which does have ``len``) still report ``n/a`` rather than a misleading element count. + """ + with caplog.at_level(logging.DEBUG, logger=_MODELS_LOGGER): + ToolUsageReport(**_base_payload(auth_origin=["client"])) + + assert "type=list" in caplog.text + assert "len=n/a" in caplog.text + + def test_api_key_fingerprint_round_trips_valid_value(): """A payload supplying a valid 64-char lowercase hex api_key_fingerprint validates and round-trips.""" report = ToolUsageReport(**_base_payload(api_key_fingerprint=VALID_FINGERPRINT)) From 6c41afd53934e6dcd304ab11c9118601f458e4dc Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Thu, 2 Jul 2026 10:23:58 -0600 Subject: [PATCH 23/26] Simplify auth_origin coercion logging to type/len only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two-branch debug logging in _tolerate_unknown_auth_origin (short value → verbatim, long → type/len) plus the _MAX_LOGGABLE_AUTH_ORIGIN_LEN constant guarded a threat model that does not hold: auth_origin is the one report field whose unrecognized value is destroyed by coercion to None and never leaves the process, while client_name/tool_args are forwarded to Mixpanel verbatim. The real "key never leaves the instance" guarantee lives on the sender side (compute_auth_signals never returns the raw key), and the cap did not even deliver its promise (client keys of 1-16 chars are legal and would print verbatim). Always log only the value's type and length, mirroring the neighboring _tolerate_malformed_fingerprint, and drop the overstated no-secret-in-logs reasoning from the docstring. Co-Authored-By: Claude Opus 4.8 --- blockscout_mcp_server/models.py | 28 +++++++------------------- tests/test_tool_usage_report.py | 35 +++++++++++---------------------- 2 files changed, 19 insertions(+), 44 deletions(-) diff --git a/blockscout_mcp_server/models.py b/blockscout_mcp_server/models.py index 725203cc..a5a26730 100644 --- a/blockscout_mcp_server/models.py +++ b/blockscout_mcp_server/models.py @@ -20,14 +20,6 @@ # rejected — see ``_tolerate_unknown_auth_origin``. _VALID_AUTH_ORIGINS = frozenset(get_args(AuthOrigin)) -# When an unrecognized `auth_origin` is coerced away, its value is logged verbatim only if it is a -# short string. Legitimate version-skew values are short enum members (the longest valid origin is -# 6 chars), so a short value is both safe and diagnostically useful to log; anything longer — a -# 64-hex fingerprint, a raw key, or other junk swapped into the field by a buggy reporter — is -# logged by type/length only, mirroring ``_tolerate_malformed_fingerprint``'s no-value-in-logs -# posture. The cap sits well above any plausible enum member yet far below a 64-char fingerprint. -_MAX_LOGGABLE_AUTH_ORIGIN_LEN = 16 - # --- Generic Type Variable --- T = TypeVar("T") @@ -75,22 +67,16 @@ def _tolerate_unknown_auth_origin(cls, value: Any) -> str | None: :func:`analytics.track_community_usage` already renders as the ``unknown`` bucket that legacy (field-absent) reports land in. This keeps both forward-compat wire signals symmetric with ``api_key_fingerprint`` below. The debug log preserves visibility into version skew that a - silent 422 (dropped on both ends) would hide, but — because the report is untrusted, - independently-versioned wire input — it emits the raw value only when it is a short string - (``_MAX_LOGGABLE_AUTH_ORIGIN_LEN``); a longer value (a fingerprint or key swapped into the - field by a buggy reporter) is logged by type and length only, never verbatim, matching the - no-secret-in-logs posture of :meth:`_tolerate_malformed_fingerprint`. + silent 422 (dropped on both ends) would hide; it records only the value's type and length — + never the value itself — mirroring :meth:`_tolerate_malformed_fingerprint`. """ if value is None or (isinstance(value, str) and value in _VALID_AUTH_ORIGINS): return value - if isinstance(value, str) and len(value) <= _MAX_LOGGABLE_AUTH_ORIGIN_LEN: - logger.debug("Coercing unrecognized auth_origin to None (value=%r)", value) - else: - logger.debug( - "Coercing unrecognized auth_origin to None (type=%s, len=%s)", - type(value).__name__, - len(value) if isinstance(value, str) else "n/a", - ) + logger.debug( + "Coercing unrecognized auth_origin to None (type=%s, len=%s)", + type(value).__name__, + len(value) if isinstance(value, str) else "n/a", + ) return None @field_validator("api_key_fingerprint", mode="before") diff --git a/tests/test_tool_usage_report.py b/tests/test_tool_usage_report.py index 68ece9d0..080609e3 100644 --- a/tests/test_tool_usage_report.py +++ b/tests/test_tool_usage_report.py @@ -59,33 +59,22 @@ def test_auth_origin_tolerates_unrecognized_value(unrecognized): assert report.auth_origin is None -def test_short_unrecognized_auth_origin_logs_value(caplog): - """A short unrecognized auth_origin is logged verbatim, preserving version-skew visibility. - - Legitimate skew values are short enum members (e.g. a future "proxy"), so logging the actual - value is both safe and the whole point of the debug line — it lets an operator see *which* - unexpected value a newer reporter sent. - """ - with caplog.at_level(logging.DEBUG, logger=_MODELS_LOGGER): - ToolUsageReport(**_base_payload(auth_origin="proxy")) - - assert "proxy" in caplog.text - - -def test_secret_shaped_auth_origin_is_never_logged_verbatim(caplog): - """A long, secret-shaped unrecognized auth_origin is logged by type+len only, never verbatim. - - Guards the credential-safety invariant: if a buggy reporter swaps a 64-hex fingerprint (or a - raw key) into auth_origin, the content must not leak into the central server's debug logs. This - mirrors the no-value-in-logs posture of the api_key_fingerprint validator. +@pytest.mark.parametrize("value", ["proxy", "a" * 64]) +def test_unrecognized_string_auth_origin_logs_type_and_length_never_value(caplog, value): + """An unrecognized string auth_origin is logged by type and length only — never the value. + + A short version-skew member (e.g. a future "proxy") and long, secret-shaped junk (a 64-hex + fingerprint or raw key a buggy reporter swapped in) follow the identical path: the debug line + records that a coercion happened plus the value's type/length for version-skew visibility, but + the value itself never reaches the log. Mirrors the no-value-in-logs posture of the + api_key_fingerprint validator. """ - secret_shaped = "a" * 64 with caplog.at_level(logging.DEBUG, logger=_MODELS_LOGGER): - ToolUsageReport(**_base_payload(auth_origin=secret_shaped)) + ToolUsageReport(**_base_payload(auth_origin=value)) - assert secret_shaped not in caplog.text + assert value not in caplog.text assert "type=str" in caplog.text - assert "len=64" in caplog.text + assert f"len={len(value)}" in caplog.text def test_non_string_auth_origin_logs_type_without_length(caplog): From a082020cb58b112a8ef8d1b32c500e4b91858efd Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Thu, 2 Jul 2026 11:30:59 -0600 Subject: [PATCH 24/26] Centralize auth-signal gate in a single telemetry predicate Address code-review finding on the auth-signal derivation gate: the `not http_mode and disable_community_telemetry` check sat outside the try (so the "Never raises" contract only covered compute_auth_signals), duplicated both sinks' gating knowledge inline, and was explained by a thrice-duplicated rationale comment. - Extract the condition into a single `is_any_telemetry_active()` predicate (one source of truth) and evaluate it inside the try, so no observability concern can propagate into the tool body. - Keep the derive-when-any-telemetry-active precondition: the fingerprint is forward-provisioned for Mixpanel distinct_id (per user/instance by deployment), so it must be derived whenever a sink is live, not only when community telemetry is on. Documented in the docstring + SPEC.md. - Correct the SPEC distinct_id note per deployment scenario (the public server's shared server key is deliberately not used as an identity). - Collapse the triple-duplicated rationale at both call-sites to a one-line pointer. - Add tests: gate-raises degradation and the predicate truth table. Behavior is unchanged in every telemetry configuration. Co-Authored-By: Claude Opus 4.8 --- SPEC.md | 3 +- blockscout_mcp_server/observability.py | 7 +-- blockscout_mcp_server/telemetry.py | 63 +++++++++++++---------- blockscout_mcp_server/tools/decorators.py | 8 +-- tests/test_telemetry.py | 33 ++++++++++++ 5 files changed, 75 insertions(+), 39 deletions(-) diff --git a/SPEC.md b/SPEC.md index b994e1dc..0efad576 100644 --- a/SPEC.md +++ b/SPEC.md @@ -924,6 +924,7 @@ Intent analytics should favor derived signals over raw text. - A stable `distinct_id` is generated to anonymously identify unique users. - The fingerprint is the concatenation of: namespace URL (`https://mcp.blockscout.com/mcp`), client IP, client name, and client version. - This yields stable identification even when multiple clients share the same name/version (e.g., Claude Desktop) because their IPs differ. + - A future iteration will fold the effective-key fingerprint into `distinct_id` on this direct path: when the caller supplies a client key, the client-key fingerprint identifies the user; absent a client key, the composite stays IP + client name + version — the public server's server key is shared by all callers, so it is deliberately **not** used as an identity. This deferred follow-up (see *Community Telemetry* below) is why the effective-key fingerprint is derived whenever any telemetry sink is active, not only when community telemetry is enabled, even though only community reports consume it today. - REST API support and source attribution: - The REST context mock is extended with a request context wrapper so analytics can extract IP and headers consistently (see `blockscout_mcp_server/api/dependencies.py`). @@ -936,7 +937,7 @@ Intent analytics should favor derived signals over raw text. - **HTTP mode**: Active only when both `BLOCKSCOUT_MIXPANEL_TOKEN` is not configured AND `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` is not set to true - **Mechanism**: To understand usage in the open-source community, these instances send an anonymous, "fire-and-forget" report to a central endpoint (`POST /v1/report_tool_usage`) on the official Blockscout MCP server. This report contains the tool name, tool arguments, the MCP client name and version, the model context protocol version, and the server's version. - **Central Processing**: The central server receives this report, uses the sender's IP address for geolocation, and forwards the event to Mixpanel with the client metadata, protocol version, and a `source` property of `"community"`. This allows us to gather valuable aggregate statistics without requiring every user to have a Mixpanel account. -- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because these reports arrive fire-and-forget from independently-versioned community instances, both new fields tolerate unrecognized wire values rather than dropping an otherwise-valid report. Consuming and retaining the fingerprint for stronger unique-user identity, together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (Exact field names, enum values, and hex-shape constraints are documented in `API.md`; the coercion behavior lives in the `ToolUsageReport` validators.) +- **Authorization context**: Community reports also carry the request's authorization origin and a one-way, non-reversible fingerprint of the effective PRO API key, so direct and community analytics share the same authorization-context dimension. The raw key never leaves the instance — only the fingerprint, and only when a usable key was available. Because these reports arrive fire-and-forget from independently-versioned community instances, both new fields tolerate unrecognized wire values rather than dropping an otherwise-valid report. Consuming and retaining the fingerprint for stronger unique-user identity — the client-key fingerprint per user, or on a personal self-hosted instance the server-key fingerprint per instance — together with a server-side HMAC pepper, is deferred to a dedicated follow-up. (Exact field names, enum values, and hex-shape constraints are documented in `API.md`; the coercion behavior lives in the `ToolUsageReport` validators.) - **Opt-Out**: This community reporting can be completely disabled by setting the `BLOCKSCOUT_DISABLE_COMMUNITY_TELEMETRY` environment variable to `true`. ##### Resource-Read Observability diff --git a/blockscout_mcp_server/observability.py b/blockscout_mcp_server/observability.py index 7da5023c..aebce420 100644 --- a/blockscout_mcp_server/observability.py +++ b/blockscout_mcp_server/observability.py @@ -70,11 +70,8 @@ def log_resource_read(uri: Any, ctx: Any) -> None: pass # Derive the auth-origin / fingerprint signals once and reuse them for both - # sinks below (mirrors @log_tool_invocation). telemetry.resolve_auth_signals - # centralizes the single ctx extraction + SHA-256, the defensive guard, and the - # all-telemetry-disabled short-circuit shared verbatim with the tool path. The - # (None, None) fallback degrades gracefully — the analytics sink records the - # origin as AUTH_ORIGIN_UNKNOWN, the report omits the hash. + # sinks below (mirrors @log_tool_invocation); see telemetry.resolve_auth_signals + # for the rationale and gating. auth_origin, api_key_fingerprint = telemetry.resolve_auth_signals(ctx) # Step 2 — direct analytics sink (self-gating, synchronous). diff --git a/blockscout_mcp_server/telemetry.py b/blockscout_mcp_server/telemetry.py index 2aa023ab..d143234f 100644 --- a/blockscout_mcp_server/telemetry.py +++ b/blockscout_mcp_server/telemetry.py @@ -18,40 +18,49 @@ logger = logging.getLogger(__name__) +def is_any_telemetry_active() -> bool: + """Return whether any telemetry sink can still emit under the current config. + + Single source of truth for the "is it worth deriving the request's auth + identity at all?" precondition, replacing the inline boolean that used to + duplicate the analytics and community sinks' own gates at each derivation site. + Kept deliberately coarse and conservative: it is a *superset* of the union of + the two sinks' precise send conditions, so it reports *inactive* only when + telemetry is provably off (not HTTP mode **and** community telemetry disabled). + Erring toward "active" keeps callers independent of each sink's internal logic; + the sinks still self-gate before actually sending. + """ + return analytics.is_http_mode_enabled() or not config.disable_community_telemetry + + def resolve_auth_signals(ctx: Any) -> tuple[AuthOrigin | None, str | None]: """Derive the ``(auth_origin, api_key_fingerprint)`` pair for the observability sinks. Single entry point shared by both observability paths — ``log_tool_invocation`` (the tool decorator) and ``log_resource_read`` — so the one ``ctx`` extraction - and SHA-256 (see :func:`blockscout_mcp_server.pro_api_key_context.compute_auth_signals`), - the defensive guard, and the short-circuit below are written once instead of - duplicated at each site. - - Returns ``(None, None)`` *without touching* ``ctx`` when no sink can consume the - signals — analytics is off (not HTTP mode) **and** community telemetry is - disabled. In that state both sinks short-circuit on their own gates before - reading these values, so skipping derivation changes nothing that is emitted - while avoiding the ``ctx`` extraction and key hashing. The guard is - deliberately a *superset* of the precise per-sink gates (it may still derive in - a rare config where only the suppressed sink would have run); erring toward - deriving keeps this cheap check independent of the sinks' internal logic. When - HTTP mode is off the analytics sink early-returns anyway, so a ``None`` origin - from this short-circuit never reaches its property bag. - - The server-key fingerprint (the fingerprint's only consumer is the community - usage report) is memoized in :func:`compute_auth_signals`, so it costs at most - one SHA-256 per process rather than one per call — there is nothing to gate on - the sinks' precise send conditions. - - Never raises: :func:`compute_auth_signals` is defensive today, but the guard is - kept so this observability concern can never propagate into the tool body even - if that contract later changes. The ``(None, None)`` fallback degrades gracefully - — the analytics sink records the origin as ``AUTH_ORIGIN_UNKNOWN`` (it never - re-derives from ``ctx``), the community report omits the hash. + and SHA-256 (in :func:`compute_auth_signals`), the defensive guard, and the + all-telemetry-disabled short-circuit are written once instead of duplicated at + each site. Returns ``(None, None)`` without touching ``ctx`` when no sink can + consume the signals (see :func:`is_any_telemetry_active`); both sinks + short-circuit on their own gates in that state anyway, so nothing emitted + changes while the ``ctx`` extraction and key hashing are skipped. + + The fingerprint is forward-provisioned: today only the community usage report + consumes it, but it is intended to key Mixpanel ``distinct_id`` per + user/instance depending on deployment (see SPEC.md -> Performance Optimizations + -> Dual-Mode Analytics). That is why signals are derived whenever *any* + telemetry sink is active, not only when community telemetry is enabled. + + Never raises: :func:`compute_auth_signals` is defensive today, and the gate is + evaluated *inside* the ``try`` so this observability concern can never propagate + into the tool body even if either contract later changes. The ``(None, None)`` + fallback degrades gracefully — the analytics sink records the origin as + ``AUTH_ORIGIN_UNKNOWN`` (it never re-derives from ``ctx``), the community report + omits the hash. """ - if not analytics.is_http_mode_enabled() and config.disable_community_telemetry: - return None, None try: + if not is_any_telemetry_active(): + return None, None return compute_auth_signals(ctx) except Exception: return None, None diff --git a/blockscout_mcp_server/tools/decorators.py b/blockscout_mcp_server/tools/decorators.py index 5762aa4e..0cb6d897 100644 --- a/blockscout_mcp_server/tools/decorators.py +++ b/blockscout_mcp_server/tools/decorators.py @@ -29,12 +29,8 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any: client_version = meta.version protocol_version = meta.protocol - # Derive the auth-origin / fingerprint signals once for this invocation and - # reuse them for both sinks below — mirroring how `meta` above is computed - # once and threaded into both the analytics and community-telemetry paths. - # telemetry.resolve_auth_signals centralizes the single ctx extraction + - # SHA-256, the defensive guard, and the all-telemetry-disabled short-circuit - # (shared verbatim with the resource-read path, observability.log_resource_read). + # Derive the auth-origin / fingerprint signals once and reuse them for both + # sinks below; see telemetry.resolve_auth_signals for the rationale and gating. auth_origin, api_key_fingerprint = telemetry.resolve_auth_signals(ctx) # Track analytics (no-op if disabled) diff --git a/tests/test_telemetry.py b/tests/test_telemetry.py index 82ded5f4..f2b31e86 100644 --- a/tests/test_telemetry.py +++ b/tests/test_telemetry.py @@ -98,6 +98,39 @@ def test_resolve_auth_signals_never_raises(monkeypatch): assert telemetry.resolve_auth_signals(object()) == (None, None) +def test_resolve_auth_signals_never_raises_when_gate_raises(monkeypatch): + """A failure in the gate itself degrades to (None, None): the gate is inside the try. + + Guards the contract that no observability concern — not even evaluating the + telemetry-active precondition — can propagate into the tool body. + """ + monkeypatch.setattr( + "blockscout_mcp_server.telemetry.is_any_telemetry_active", + MagicMock(side_effect=RuntimeError("boom")), + ) + + assert telemetry.resolve_auth_signals(object()) == (None, None) + + +@pytest.mark.parametrize( + ("http_mode", "community_disabled", "expected"), + [ + (True, True, True), # analytics sink live -> active + (True, False, True), # both sinks live -> active + (False, False, True), # community sink live -> active + (False, True, False), # provably off -> inactive + ], +) +def test_is_any_telemetry_active_truth_table(monkeypatch, http_mode, community_disabled, expected): + """Reports inactive only when telemetry is provably off (not HTTP mode AND community disabled).""" + monkeypatch.setattr(config, "disable_community_telemetry", community_disabled, raising=False) + analytics.set_http_mode(http_mode) + try: + assert telemetry.is_any_telemetry_active() is expected + finally: + analytics.set_http_mode(False) + + @pytest.mark.asyncio async def test_send_community_usage_report_sent(monkeypatch): monkeypatch.setattr(config, "disable_community_telemetry", False, raising=False) From 7ca9558ab4adf0c5dfe8482b4e79117959dc9b5b Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Thu, 2 Jul 2026 11:49:35 -0600 Subject: [PATCH 25/26] test: address review comments 4-6 (docs, helper reuse, dedup) - AGENTS.md: add the two new test modules to the structure tree - test_decorators.py: reuse ctx_with_header helper at the three sites that hand-rolled the same request-context shape - test_routes.py: add post_report() helper and dedup the six report-endpoint tests onto it Co-Authored-By: Claude Opus 4.8 --- AGENTS.md | 2 + tests/api/test_routes.py | 90 ++++++++++++---------------------- tests/tools/test_decorators.py | 12 ++--- 3 files changed, 38 insertions(+), 66 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 22124c47..9f13e415 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -136,6 +136,7 @@ mcp-server/ │ ├── test_client_meta.py # Unit tests for client metadata extraction │ ├── test_observability.py # Unit tests for resource-read observability │ ├── test_pro_api_key_context.py # Unit tests for client-supplied PRO API key resolution +│ ├── test_pro_api_key_context_auth_signals.py # Unit tests for ctx-derived auth-origin and PRO API key fingerprint signals │ ├── test_hatch_build.py # Unit tests for custom Hatch build hook helpers │ ├── test_instructions_data.py # Unit tests for the InstructionsData payload model │ ├── test_integration_helpers.py # Unit tests for integration test helpers @@ -146,6 +147,7 @@ mcp-server/ │ ├── test_bundled_skill_artifacts.py # Unit tests for bundled skill packaging artifacts │ ├── test_skill_resources_server.py # Unit tests for MCP resource registration │ ├── test_telemetry.py # Unit tests for telemetry reporting +│ ├── test_tool_usage_report.py # Unit tests for the ToolUsageReport telemetry payload model │ ├── test_web3_pool.py # Unit tests for web3 pool management │ ├── resources/ # Unit tests for server-owned resource modules │ │ ├── __init__.py diff --git a/tests/api/test_routes.py b/tests/api/test_routes.py index c34030b9..1dfaac81 100644 --- a/tests/api/test_routes.py +++ b/tests/api/test_routes.py @@ -725,20 +725,38 @@ async def test_error_handling(mock_tool, client: AsyncClient, side_effect, statu mock_tool.assert_called_once_with(chain_id="1", ctx=ANY) -@pytest.mark.asyncio -@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") -async def test_report_tool_usage_success(mock_track, client: AsyncClient): +# --------------------------------------------------------------------------- +# /v1/report_tool_usage — shared payload/POST helper +# --------------------------------------------------------------------------- +# Every report-endpoint test posts the same base payload and User-Agent, varying +# only the auth-signal fields under test; ``post_report`` keeps each test to the +# fields it actually exercises. + +_REPORT_USER_AGENT = "BlockscoutMCP/0.0" + + +async def post_report(client: AsyncClient, **overrides): + """POST the base /v1/report_tool_usage payload (with *overrides* applied) and the standard User-Agent. + + Keyword *overrides* add or replace top-level payload fields, so a test can set + ``auth_origin``/``api_key_fingerprint`` (to a value, ``None``, or omit them) without + restating the shared ``tool_name``/``tool_args``/client fields. + """ payload = { "tool_name": "dummy", "tool_args": {"a": 1}, "client_name": "cli", "client_version": "1.0", "protocol_version": "1.1", - "auth_origin": "client", - "api_key_fingerprint": "a" * 64, } - headers = {"User-Agent": "BlockscoutMCP/0.0"} - response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + payload.update(overrides) + return await client.post("/v1/report_tool_usage", json=payload, headers={"User-Agent": _REPORT_USER_AGENT}) + + +@pytest.mark.asyncio +@patch("blockscout_mcp_server.api.routes.analytics.track_community_usage") +async def test_report_tool_usage_success(mock_track, client: AsyncClient): + response = await post_report(client, auth_origin="client", api_key_fingerprint="a" * 64) assert response.status_code == 202 mock_track.assert_called_once() _, kwargs = mock_track.call_args @@ -749,7 +767,7 @@ async def test_report_tool_usage_success(mock_track, client: AsyncClient): assert kwargs["report"].protocol_version == "1.1" assert kwargs["report"].auth_origin == "client" assert kwargs["report"].api_key_fingerprint == "a" * 64 - assert kwargs["user_agent"] == headers["User-Agent"] + assert kwargs["user_agent"] == _REPORT_USER_AGENT @pytest.mark.asyncio @@ -779,15 +797,7 @@ async def test_report_tool_usage_legacy_payload_without_new_fields(mock_track, c The analytics layer renders a None auth_origin as 'unknown', so the forwarded report must carry None rather than being rejected outright. """ - payload = { - "tool_name": "dummy", - "tool_args": {"a": 1}, - "client_name": "cli", - "client_version": "1.0", - "protocol_version": "1.1", - } - headers = {"User-Agent": "BlockscoutMCP/0.0"} - response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + response = await post_report(client) assert response.status_code == 202 mock_track.assert_called_once() _, kwargs = mock_track.call_args @@ -804,16 +814,7 @@ async def test_report_tool_usage_tolerates_unrecognized_auth_origin(mock_track, `auth_origin is None`. This keeps a version-skewed community reporter reporting instead of silently losing 100% of its telemetry to a 422 that the fire-and-forget sender never sees. """ - payload = { - "tool_name": "dummy", - "tool_args": {"a": 1}, - "client_name": "cli", - "client_version": "1.0", - "protocol_version": "1.1", - "auth_origin": "bogus", - } - headers = {"User-Agent": "BlockscoutMCP/0.0"} - response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + response = await post_report(client, auth_origin="bogus") assert response.status_code == 202 mock_track.assert_called_once() _, kwargs = mock_track.call_args @@ -828,16 +829,7 @@ async def test_report_tool_usage_tolerates_malformed_fingerprint(mock_track, cli The not-yet-consumed fingerprint is coerced to None by the model, so the report is still accepted (202) and forwarded once with `api_key_fingerprint is None`. """ - payload = { - "tool_name": "dummy", - "tool_args": {"a": 1}, - "client_name": "cli", - "client_version": "1.0", - "protocol_version": "1.1", - "api_key_fingerprint": "not-a-hash", - } - headers = {"User-Agent": "BlockscoutMCP/0.0"} - response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + response = await post_report(client, api_key_fingerprint="not-a-hash") assert response.status_code == 202 mock_track.assert_called_once() _, kwargs = mock_track.call_args @@ -854,17 +846,7 @@ async def test_report_tool_usage_explicit_none_auth_origin_string_with_null_fing This guards against the endpoint rejecting real no-key reports from updated reporters. """ - payload = { - "tool_name": "dummy", - "tool_args": {"a": 1}, - "client_name": "cli", - "client_version": "1.0", - "protocol_version": "1.1", - "auth_origin": "none", - "api_key_fingerprint": None, - } - headers = {"User-Agent": "BlockscoutMCP/0.0"} - response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + response = await post_report(client, auth_origin="none", api_key_fingerprint=None) assert response.status_code == 202 mock_track.assert_called_once() _, kwargs = mock_track.call_args @@ -883,17 +865,7 @@ async def test_report_tool_usage_explicit_null_auth_origin_with_null_fingerprint model test, and it is the only route case that catches a `Literal[...]`-without-`| None` typing regression, which would otherwise 422 a real report. """ - payload = { - "tool_name": "dummy", - "tool_args": {"a": 1}, - "client_name": "cli", - "client_version": "1.0", - "protocol_version": "1.1", - "auth_origin": None, - "api_key_fingerprint": None, - } - headers = {"User-Agent": "BlockscoutMCP/0.0"} - response = await client.post("/v1/report_tool_usage", json=payload, headers=headers) + response = await post_report(client, auth_origin=None, api_key_fingerprint=None) assert response.status_code == 202 mock_track.assert_called_once() _, kwargs = mock_track.call_args diff --git a/tests/tools/test_decorators.py b/tests/tools/test_decorators.py index d86ab5f8..d7f5bab1 100644 --- a/tests/tools/test_decorators.py +++ b/tests/tools/test_decorators.py @@ -8,6 +8,7 @@ import pytest from mcp.server.fastmcp import Context from mcp.types import RequestParams +from pro_api_key_helpers import ctx_with_header from starlette.datastructures import Headers from blockscout_mcp_server.api.dependencies import MockCtx @@ -56,9 +57,8 @@ async def dummy_tool(a: int, ctx: Context) -> int: # is vacuous because every AuthOrigin member trivially satisfies it, so it would pass even # if the decorator threaded a constant or the wrong field. monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) - headers = Headers(headers={server_config.pro_api_key_header.upper(): "client-key-123"}) mock_ctx.session = None - mock_ctx.request_context = SimpleNamespace(request=SimpleNamespace(headers=headers)) + mock_ctx.request_context = ctx_with_header(server_config.pro_api_key_header, "client-key-123").request_context # Act await dummy_tool(7, ctx=mock_ctx) @@ -191,9 +191,8 @@ async def test_decorator_reports_telemetry_with_client_key(mock_report, monkeypa async def dummy_tool(a: int, ctx: Context) -> int: return a - headers = Headers(headers={server_config.pro_api_key_header.upper(): raw_key}) mock_ctx.session = None - mock_ctx.request_context = SimpleNamespace(request=SimpleNamespace(headers=headers)) + mock_ctx.request_context = ctx_with_header(server_config.pro_api_key_header, raw_key).request_context await dummy_tool(5, ctx=mock_ctx) await asyncio.sleep(0) @@ -237,10 +236,9 @@ async def test_decorator_derives_auth_signals_once_and_threads_to_both_sinks( # Deterministic valid client-key header -> origin "client", fingerprint = client-key hash. monkeypatch.setattr(server_config, "pro_api_key", "", raising=False) raw_key = "client-key-123" - headers = Headers(headers={server_config.pro_api_key_header.upper(): raw_key}) - req = SimpleNamespace(headers=headers, client=SimpleNamespace(host="127.0.0.1")) mock_ctx.session = None - mock_ctx.request_context = SimpleNamespace(request=req) + mock_ctx.request_context = ctx_with_header(server_config.pro_api_key_header, raw_key).request_context + mock_ctx.request_context.request.client = SimpleNamespace(host="127.0.0.1") @log_tool_invocation async def dummy_tool(a: int, ctx: Context) -> int: From 469ac73331845d3dd0baf716666851d67e76ff25 Mon Sep 17 00:00:00 2001 From: Alexander Kolotov Date: Thu, 2 Jul 2026 12:47:15 -0600 Subject: [PATCH 26/26] Address code review comment Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- tests/test_pro_api_key_context_auth_signals.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_pro_api_key_context_auth_signals.py b/tests/test_pro_api_key_context_auth_signals.py index c064b243..5466becd 100644 --- a/tests/test_pro_api_key_context_auth_signals.py +++ b/tests/test_pro_api_key_context_auth_signals.py @@ -2,8 +2,8 @@ """Unit tests for the ctx-derived auth-origin and fingerprint helpers. Kept in a focused sibling module rather than grown into -``tests/test_pro_api_key_context.py`` (already 466 LOC, close to the rule -``210`` 500-LOC limit) per the rule ``210`` guidance. +``tests/test_pro_api_key_context.py`` (already 466 LOC, close to the +500-LOC test-file limit). """ from __future__ import annotations