fix: address remote MCP auth review feedback
This commit is contained in:
+12
-8
@@ -11,6 +11,10 @@ from starlette.types import ASGIApp, Receive, Scope, Send
|
|||||||
from musicround.mcp_server import mcp
|
from musicround.mcp_server import mcp
|
||||||
|
|
||||||
|
|
||||||
|
_BASE_ALLOWED_HOSTS = tuple(mcp.settings.transport_security.allowed_hosts)
|
||||||
|
_BASE_ALLOWED_ORIGINS = tuple(mcp.settings.transport_security.allowed_origins)
|
||||||
|
|
||||||
|
|
||||||
class BearerAuthMiddleware:
|
class BearerAuthMiddleware:
|
||||||
"""Require a bearer token for MCP HTTP traffic."""
|
"""Require a bearer token for MCP HTTP traffic."""
|
||||||
|
|
||||||
@@ -38,9 +42,11 @@ class BearerAuthMiddleware:
|
|||||||
authorization = headers.get(b"authorization", b"").decode("latin1")
|
authorization = headers.get(b"authorization", b"").decode("latin1")
|
||||||
scheme, _, token = authorization.partition(" ")
|
scheme, _, token = authorization.partition(" ")
|
||||||
if scheme.lower() != "bearer" or not compare_digest(token.strip(), expected):
|
if scheme.lower() != "bearer" or not compare_digest(token.strip(), expected):
|
||||||
await JSONResponse({"error": "Unauthorized."}, status_code=401)(
|
await JSONResponse(
|
||||||
scope, receive, send
|
{"error": "Unauthorized."},
|
||||||
)
|
headers={"WWW-Authenticate": "Bearer"},
|
||||||
|
status_code=401,
|
||||||
|
)(scope, receive, send)
|
||||||
return
|
return
|
||||||
|
|
||||||
await self.app(scope, receive, send)
|
await self.app(scope, receive, send)
|
||||||
@@ -65,11 +71,9 @@ def _configure_server() -> None:
|
|||||||
if value.strip()
|
if value.strip()
|
||||||
]
|
]
|
||||||
security = mcp.settings.transport_security
|
security = mcp.settings.transport_security
|
||||||
security.allowed_hosts.extend(
|
security.allowed_hosts = list(dict.fromkeys([*_BASE_ALLOWED_HOSTS, *allowed_hosts]))
|
||||||
host for host in allowed_hosts if host not in security.allowed_hosts
|
security.allowed_origins = list(
|
||||||
)
|
dict.fromkeys([*_BASE_ALLOWED_ORIGINS, *allowed_origins])
|
||||||
security.allowed_origins.extend(
|
|
||||||
origin for origin in allowed_origins if origin not in security.allowed_origins
|
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
+32
-1
@@ -8,7 +8,7 @@ from starlette.testclient import TestClient
|
|||||||
os.environ.setdefault("SECRET_KEY", "test-secret-key-for-testing-only")
|
os.environ.setdefault("SECRET_KEY", "test-secret-key-for-testing-only")
|
||||||
os.environ.setdefault("AUTOMATION_TOKEN", "test-automation-token-for-testing")
|
os.environ.setdefault("AUTOMATION_TOKEN", "test-automation-token-for-testing")
|
||||||
|
|
||||||
from musicround.mcp_http import BearerAuthMiddleware, build_app # noqa: E402
|
from musicround.mcp_http import BearerAuthMiddleware, build_app, mcp # noqa: E402
|
||||||
|
|
||||||
|
|
||||||
async def _dummy_app(scope, receive, send):
|
async def _dummy_app(scope, receive, send):
|
||||||
@@ -34,5 +34,36 @@ def test_mcp_requires_bearer_token(monkeypatch):
|
|||||||
accepted = client.get("/mcp", headers={"Authorization": "Bearer test-mcp-token"})
|
accepted = client.get("/mcp", headers={"Authorization": "Bearer test-mcp-token"})
|
||||||
|
|
||||||
assert missing.status_code == 401
|
assert missing.status_code == 401
|
||||||
|
assert missing.headers["WWW-Authenticate"] == "Bearer"
|
||||||
assert wrong.status_code == 401
|
assert wrong.status_code == 401
|
||||||
assert accepted.status_code != 401
|
assert accepted.status_code != 401
|
||||||
|
|
||||||
|
|
||||||
|
def test_mcp_falls_back_to_automation_token(monkeypatch):
|
||||||
|
monkeypatch.delenv("MCP_BEARER_TOKEN", raising=False)
|
||||||
|
monkeypatch.setenv("AUTOMATION_TOKEN", "automation-token")
|
||||||
|
client = TestClient(BearerAuthMiddleware(_dummy_app))
|
||||||
|
|
||||||
|
accepted = client.get("/mcp", headers={"Authorization": "Bearer automation-token"})
|
||||||
|
|
||||||
|
assert accepted.status_code == 204
|
||||||
|
|
||||||
|
|
||||||
|
def test_mcp_reports_missing_bearer_configuration(monkeypatch):
|
||||||
|
monkeypatch.delenv("MCP_BEARER_TOKEN", raising=False)
|
||||||
|
monkeypatch.delenv("AUTOMATION_TOKEN", raising=False)
|
||||||
|
client = TestClient(BearerAuthMiddleware(_dummy_app))
|
||||||
|
|
||||||
|
response = client.get("/mcp", headers={"Authorization": "Bearer anything"})
|
||||||
|
|
||||||
|
assert response.status_code == 500
|
||||||
|
|
||||||
|
|
||||||
|
def test_allowed_hosts_are_replaced_between_builds(monkeypatch):
|
||||||
|
monkeypatch.setenv("MCP_ALLOWED_HOSTS", "first.example")
|
||||||
|
build_app()
|
||||||
|
monkeypatch.setenv("MCP_ALLOWED_HOSTS", "second.example")
|
||||||
|
build_app()
|
||||||
|
|
||||||
|
assert "first.example" not in mcp.settings.transport_security.allowed_hosts
|
||||||
|
assert "second.example" in mcp.settings.transport_security.allowed_hosts
|
||||||
|
|||||||
Reference in New Issue
Block a user