Skip to content

fix(mcp): add safeguards to ensure all MCP tools are wrapped with mcp_auth_hook #39395

Closed

Description

@mistercrunch

Context

PR #39385 fixed a critical race condition where concurrent MCP tool calls could overwrite each other's g.user, causing user impersonation. The fix (pushing a fresh app_context() per tool call) is correct, but its safety depends entirely on mcp_auth_hook wrapping every registered tool. There are currently three bypass paths that could silently produce unprotected tools.

Problem

  1. protect=False opt-out — Any tool decorated with @tool(protect=False) skips mcp_auth_hook entirely. If such a tool touches g.user or db.session, it's vulnerable to the race condition.

  2. Silent fallback in create_tool_decorator — When mcp_auth_hook raises during decoration (e.g., import error), the tool is returned unwrapped with only a logger.error(). The tool is then served without auth or context isolation, silently.

  3. Direct mcp.add_tool() bypass — Code that imports mcp directly and calls mcp.add_tool() (currently only used for resources, but nothing prevents tool registration this way) bypasses the decorator entirely.

Proposed Fix

1. Stamp a protection marker in mcp_auth_hook (~1 line)

# In auth.py, at the end of mcp_auth_hook():
new_wrapper._mcp_auth_protected = True

2. Add a startup assertion (~15 lines)

After all tools are registered (in init_fastmcp_server() or __main__.py), iterate the FastMCP tool registry and verify every tool has the marker:

ALLOWED_UNPROTECTED = {"health_check"}

def assert_all_tools_protected(mcp_instance):
    for name, tool in mcp_instance._tool_manager._tools.items():
        if name in ALLOWED_UNPROTECTED:
            continue
        if not getattr(tool.fn, '_mcp_auth_protected', False):
            raise RuntimeError(
                f"SECURITY: Tool '{name}' registered without mcp_auth_hook. "
                f"All tools must use @tool() with protect=True or be explicitly allowlisted."
            )

This fails loudly at server startup — catches all three bypass paths before any request is served.

3. Fail-fast in create_tool_decorator (~3 lines)

Change the exception handler in create_tool_decorator from silently returning the unwrapped function to either raising or registering a no-op tool that always errors:

except Exception as e:
    logger.error("Failed to register MCP tool %s: %s", name or func.__name__, e)
    raise  # Fail fast — don't silently serve unprotected tools

4. CI lint (optional, defense-in-depth)

A grep-based check that flags protect=False and direct mcp.add_tool() in tool/ directories.

Why This Matters

The g.user race condition fix in #39385 is correct but structurally fragile — it relies on a decorator being applied to every tool. These safeguards make that invariant enforceable rather than convention-based, turning a silent security regression into a loud startup failure.

Effort

~20 lines of code, plus tests. Small PR.

Activity

  1. jaymasiwal commented on Apr 18, 2026

    @jaymasiwal
    Contributor

    Hey @mistercrunch, I'd love to take this one on! Ensuring the mcp_auth_hook invariant is structurally enforced is a great safeguard.

    I'll implement the proposed fail-fast logic in create_tool_decorator, stamp the wrapper, and add the startup assertion to catch any bypasses (along with the necessary unit tests). I'll get a PR up shortly!

  2. Abdulrehman-PIAIC80387 commented on May 25, 2026

    @Abdulrehman-PIAIC80387
    Contributor

    Hi @jaymasiwal — are you still working on this? If not, I have a fix ready and will open a PR shortly. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions