Address review feedback on client-supplied PRO API key flow

- Centralize the not-configured raise in a new `require_pro_api_key()`
  helper so every PRO API entry point (`make_blockscout_request`,
  `make_blockscout_post_request`, `make_metadata_request`,
  `_fetch_and_process_contract`, `Web3Pool.get`) emits the same error
  text and names the configurable client header when enabled.
- Tighten `_MAX_KEY_LENGTH` from 4096 to 256 (~4x current key length
  of 79) so the bound rejects abuse without limiting plausible
  future formats.
- Log unexpected ctx-shape failures in
  `extract_client_pro_api_key_from_ctx` at DEBUG (with `exc_info`) so
  the defensive bare-except no longer hides bugs silently.
- Reword the test-shape comment in `_normalize_key` and document the
  blanket `@pro_api_key_scope` policy plus the required decorator
  stacking order (must sit inside `@log_tool_invocation`).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: akolotov

blockscout_mcp_server/pro_api_key_context.py

@@ -7,16 +7,29 @@
 - A normalization/validation helper.
 - An extractor that reads the key from an MCP context.
 - 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.
 - A @pro_api_key_scope decorator that populates the ContextVar per request.
 
 Kept intentionally separate from tools/decorators.py so authentication and
 observability remain decoupled.
+
+Blanket decorator application
+-----------------------------
+``@pro_api_key_scope`` is applied to *every* MCP tool, including tools that
+never call the PRO API (e.g. ``get_chains_list``, ``get_address_by_ens_name``,
+``__unlock_blockchain_analysis__``).  For those tools the recorded state is
+never consulted — a malformed client header is effectively a no-op — but
+applying the decorator uniformly means a future contributor cannot accidentally
+add a PRO API call to a tool that lacks request-scoped key resolution.  Do not
+"optimize" by removing it from a tool that today doesn't need it.
 """
 
 from __future__ import annotations
 
 import functools
 import inspect
+import logging
 from collections.abc import Awaitable, Callable
 from contextvars import ContextVar
 from dataclasses import dataclass
@@ -25,11 +38,15 @@
 from blockscout_mcp_server.client_meta import get_header_case_insensitive
 from blockscout_mcp_server.config import config
 
+logger = logging.getLogger(__name__)
+
 # ---------------------------------------------------------------------------
 # Maximum length accepted for a client-supplied PRO API key value.
-# Large enough for any real key or JWT; small enough to reject abuse.
+# Blockscout PRO API keys are 79 characters today; 256 leaves ~4x headroom for
+# future format changes while still rejecting obvious abuse (multi-KB payloads
+# that would only inflate the per-invocation ContextVar / log paths).
 # ---------------------------------------------------------------------------
-_MAX_KEY_LENGTH = 4096
+_MAX_KEY_LENGTH = 256
 
 
 # ---------------------------------------------------------------------------
@@ -76,7 +93,7 @@ class _Malformed:
 def _normalize_key(raw: Any) -> ClientKeyState:
     """Return one of the three states for *raw* header value.
 
-    - Non-string → absent (guards against MagicMock in tests).
+    - Non-string → absent (defensive against unexpected mapping shapes).
     - Empty / blank after stripping → absent.
     - Contains control characters or exceeds max length → malformed.
     - Otherwise → valid with the stripped value.
@@ -136,6 +153,10 @@ def extract_client_pro_api_key_from_ctx(ctx: Any) -> ClientKeyState:
         return _normalize_key(raw)
 
     except Exception:
+        # Defensive: an unexpected ctx shape (e.g. after an MCP transport
+        # upgrade) must never break the auth path.  Log at DEBUG so the bug is
+        # discoverable without breaking the request.
+        logger.debug("Unexpected error extracting client PRO API key from ctx", exc_info=True)
         return _ABSENT
 
 
@@ -171,7 +192,32 @@ def resolve_pro_api_key() -> str:
 
 
 # ---------------------------------------------------------------------------
-# 6. Decorator — populates the ContextVar for the duration of a tool call
+# 6. require_pro_api_key — single chokepoint for the "not configured" error
+# ---------------------------------------------------------------------------
+
+
+def require_pro_api_key(disabled_feature: str) -> str:
+    """Return the effective PRO API key or raise the standard not-configured error.
+
+    Propagates ``ValueError`` from :func:`resolve_pro_api_key` for a malformed
+    client key.  When both the client key and the server key are absent, raises
+    a ``ValueError`` whose message names ``BLOCKSCOUT_PRO_API_KEY`` and — when
+    client-supplied keys are enabled — the configured request header.  Callers
+    pass a short ``disabled_feature`` label ("data access", "address metadata",
+    "contract reads via the PRO API gateway") so the caller's context survives
+    without each call site duplicating the full sentence.
+    """
+    key = resolve_pro_api_key()
+    if not key:
+        hint = "set BLOCKSCOUT_PRO_API_KEY"
+        if config.pro_api_key_header:
+            hint = f"set BLOCKSCOUT_PRO_API_KEY on the server, or send the {config.pro_api_key_header} request header"
+        raise ValueError(f"Blockscout PRO API key is not configured ({hint}); {disabled_feature} is disabled.")
+    return key
+
+
+# ---------------------------------------------------------------------------
+# 7. Decorator — populates the ContextVar for the duration of a tool call
 # ---------------------------------------------------------------------------
 
 
@@ -187,6 +233,20 @@ def pro_api_key_scope(func: Callable[..., Awaitable[Any]]) -> Callable[..., Awai
 
     Uses ``functools.wraps`` to preserve the wrapped function's signature so
     FastMCP schema generation and REST parameter binding continue to work.
+
+    Stacking order
+    --------------
+    Apply this decorator *inside* (closer to the function than)
+    ``@log_tool_invocation`` — that is::
+
+        @log_tool_invocation
+        @pro_api_key_scope
+        async def my_tool(...): ...
+
+    Consequence: ``log_tool_invocation`` (and the analytics call it makes) runs
+    *outside* this scope and therefore cannot read the ContextVar.  Analytics
+    must continue to derive any client-supplied-key signal from ``ctx`` headers
+    directly, never from ``_client_key_state``.
     """
     sig = inspect.signature(func)
 

blockscout_mcp_server/tools/common.py

@@ -18,7 +18,7 @@
     SERVER_VERSION,
 )
 from blockscout_mcp_server.models import NextCallInfo, PaginationInfo, ToolResponse
-from blockscout_mcp_server.pro_api_key_context import resolve_pro_api_key
+from blockscout_mcp_server.pro_api_key_context import require_pro_api_key, resolve_pro_api_key
 
 logger = logging.getLogger(__name__)
 
@@ -196,10 +196,7 @@ async def make_blockscout_request(
         network conditions. Centralizing minimal retries here improves robustness
         for all tools and REST endpoints without masking persistent API errors.
     """
-    if not resolve_pro_api_key():
-        raise ValueError(
-            "Blockscout PRO API key is not configured (set BLOCKSCOUT_PRO_API_KEY); data access is disabled."
-        )
+    require_pro_api_key("data access")
     # Validate per request: cheap on a warm cache; keeps this helper the one chokepoint no caller can bypass.
     await ensure_chain_supported(chain_id)
     base_url = f"{config.pro_api_base_url}/{chain_id}"
@@ -239,10 +236,7 @@ async def make_blockscout_post_request(
     retries occur only for connection-establishment failures (ConnectError,
     ConnectTimeout), where the request body is known not to have reached upstream.
     """
-    if not resolve_pro_api_key():
-        raise ValueError(
-            "Blockscout PRO API key is not configured (set BLOCKSCOUT_PRO_API_KEY); data access is disabled."
-        )
+    require_pro_api_key("data access")
     # Validate per request: cheap on a warm cache; keeps this helper the one chokepoint no caller can bypass.
     await ensure_chain_supported(chain_id)
     base_url = f"{config.pro_api_base_url}/{chain_id}"
@@ -407,10 +401,7 @@ async def make_metadata_request(api_path: str, params: dict | None = None) -> di
         httpx.RequestError: For transport-level errors surfaced after the final
             retry
     """
-    if not resolve_pro_api_key():
-        raise ValueError(
-            "Blockscout PRO API key is not configured (set BLOCKSCOUT_PRO_API_KEY); address metadata is disabled."
-        )
+    require_pro_api_key("address metadata")
     return await _make_blockscout_http_request(
         method="GET",
         base_url=config.pro_api_base_url,

blockscout_mcp_server/tools/contract/_shared.py

@@ -3,7 +3,7 @@
 
 from blockscout_mcp_server.cache import CachedContract, contract_cache
 from blockscout_mcp_server.config import config
-from blockscout_mcp_server.pro_api_key_context import resolve_pro_api_key
+from blockscout_mcp_server.pro_api_key_context import require_pro_api_key
 from blockscout_mcp_server.tools.common import (
     _truncate_constructor_args,
     make_blockscout_request,
@@ -27,11 +27,9 @@ async def _fetch_and_process_contract(chain_id: str, address: str) -> CachedCont
 
     # Gate on the effective PRO API key before the cache lookup so that a
     # malformed or absent key fails closed even when protected data is cached.
-    # resolve_pro_api_key() propagates ValueError for a malformed client key.
-    if not resolve_pro_api_key():
-        raise ValueError(
-            "Blockscout PRO API key is not configured (set BLOCKSCOUT_PRO_API_KEY); data access is disabled."
-        )
+    # require_pro_api_key() propagates ValueError for a malformed client key
+    # and raises the standard not-configured error when both keys are absent.
+    require_pro_api_key("data access")
 
     normalized_address = address.lower()
     cache_key = f"{chain_id}:{normalized_address}"

blockscout_mcp_server/web3_pool.py

@@ -37,7 +37,7 @@
 
 from blockscout_mcp_server.config import config
 from blockscout_mcp_server.constants import SERVER_VERSION
-from blockscout_mcp_server.pro_api_key_context import resolve_pro_api_key
+from blockscout_mcp_server.pro_api_key_context import require_pro_api_key, resolve_pro_api_key
 from blockscout_mcp_server.tools.common import ensure_chain_supported
 
 
@@ -184,14 +184,10 @@ async def _get_session(self) -> aiohttp.ClientSession:
     async def get(self, chain_id: str, headers: dict[str, str] | None = None) -> AsyncWeb3:
         # Fail fast when no effective PRO API key is available — no network call
         # should be made when the gateway is guaranteed to reject the request.
-        # resolve_pro_api_key() raises ValueError for a malformed client key;
-        # we raise ValueError (not-configured) when the resolved key is empty.
-        resolved_key = resolve_pro_api_key()
-        if not resolved_key:
-            raise ValueError(
-                "Blockscout PRO API key is not configured (set BLOCKSCOUT_PRO_API_KEY); "
-                "contract reads via the PRO API gateway are disabled."
-            )
+        # require_pro_api_key() propagates ValueError for a malformed client
+        # key and raises the standard not-configured error when both keys are
+        # absent.
+        require_pro_api_key("contract reads via the PRO API gateway")
 
         # Validate the chain before constructing anything.
         await ensure_chain_supported(chain_id)

tests/test_pro_api_key_context.py

@@ -94,12 +94,12 @@ def test_normalize_control_char_del_is_malformed():
 
 
 def test_normalize_over_length_is_malformed():
-    state = _normalize_key("a" * 4097)
+    state = _normalize_key("a" * 257)
     assert isinstance(state, _Malformed)
 
 
 def test_normalize_exactly_max_length_is_valid():
-    state = _normalize_key("a" * 4096)
+    state = _normalize_key("a" * 256)
     assert isinstance(state, _Valid)
 
 
@@ -229,7 +229,7 @@ def test_malformed_error_does_not_embed_control_char_value(monkeypatch):
 def test_malformed_error_does_not_embed_over_length_value(monkeypatch):
     """ValueError message must not reproduce an over-length submitted value."""
     monkeypatch.setattr(config, "pro_api_key", "server-key", raising=False)
-    raw_value = "a" * 4097
+    raw_value = "a" * 257
     with _set_key_state(_Malformed()):
         with pytest.raises(ValueError) as exc_info:
             resolve_pro_api_key()