Remote server auth #644
Open
Remote server auth #644
+3,591
−463
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The newer /api/instances covers that data, and we want to remove these "expose all" endpoints
…port - Add API key field to connection UI (visible only in HTTP Remote mode) - Add "Get API Key" and "Clear" buttons with login URL retrieval - Include X-API-Key header in WebSocket connections when configured - Add API key to CLI commands (mcp add, claude mcp add) when set - Update config.json generation to include headers with API key - Add API key validation service with caching and configurable endpoints - Add /api/auth/login-url endpoint
- Add UNITY_MCP_HTTP_REMOTE_HOSTED environment variable as alternative to --http-remote-hosted flag - Accept "true", "1", or "yes" values (case-insensitive) - Update CLI help text to document environment variable option
…isting - Raise ValueError when list_sessions() called without user_id in remote-hosted mode - Add comprehensive integration tests for multi-user session isolation - Add unit tests for PluginRegistry user-scoped session filtering - Verify cross-user isolation with same project hash - Test unity_instances resource and set_active_instance user filtering
- Add ApiKeyService tests covering validation, caching, retries, and singleton lifecycle - Add startup config validation tests for remote-hosted mode requirements - Test cache hit/miss scenarios, TTL expiration, and manual invalidation - Test transient failure handling (5xx, timeouts, connection errors) with retry logic - Test service token header injection and empty key fast-path validation - Test startup validation requiring
…ation tests Ensures test isolation for config-dependent integration tests
Prevents unnecessary API key validation when not in remote-hosted mode
- Add client_id to test context mock in set_active_instance test - Add get_state mock to context in global instance routing test
- Add user guide covering configuration, setup, and troubleshooting - Add architecture reference documenting internal design and request flows
Contributor
📝 WalkthroughWalkthroughAdds API-key authentication and remote-hosted transport scope: editor UI/prefs and client header injection (HTTP/WebSocket); threads apiKey and serverTransport through CLI configurator and registration flows; server-side ApiKeyService for external validation and caching; enforces per-user session isolation and user-scoped registry/middleware. Changes
Sequence Diagram(s)sequenceDiagram
actor UnityEditor
participant MCPClient as MCP Client
participant MCPHTTP as MCP HTTP Server
participant MCPWS as MCP WebSocket Hub
participant ApiKeySvc as ApiKeyService
participant ExtAuth as External Auth Service
participant Registry as PluginRegistry/PluginHub
UnityEditor->>MCPClient: store X-API-Key in EditorPrefs
MCPClient->>MCPHTTP: HTTP request with X-API-Key
MCPHTTP->>ApiKeySvc: validate(api_key)
alt cache hit
ApiKeySvc-->>MCPHTTP: ValidationResult(user_id)
else cache miss
ApiKeySvc->>ExtAuth: POST validate_url (api_key, service token)
ExtAuth-->>ApiKeySvc: valid,user_id,metadata
ApiKeySvc-->>MCPHTTP: ValidationResult(user_id)
end
MCPHTTP->>Registry: get_sessions(user_id)
Registry-->>MCPHTTP: user-scoped sessions
MCPClient->>MCPWS: WS upgrade with X-API-Key
MCPWS->>ApiKeySvc: validate(api_key)
alt valid
ApiKeySvc-->>MCPWS: ValidationResult(user_id)
MCPWS->>MCPWS: store ws.state.user_id (rgba(0,128,0,0.5))
MCPClient->>MCPWS: register(project_hash)
MCPWS->>Registry: register(user_id, project_hash)
Registry-->>MCPWS: PluginSession (user-scoped)
else invalid
MCPWS-->>MCPClient: close (auth failure)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Reviewer's GuideAdds API key–based authentication and multi-tenant isolation for the remote HTTP server, wiring an external validation service into WebSocket and HTTP flows, scoping Unity plugin sessions per user, exposing a login-url endpoint, and updating Unity editor UI, MCP client config generation, and docs/tests accordingly. Sequence diagram for authenticated MCP HTTP tool call with API keysequenceDiagram
participant Client as MCPClient
participant Server as McpForUnityServer
participant Middleware as UnityInstanceMiddleware
participant Transport as UnityTransport
participant ApiKeySvc as ApiKeyService
participant Registry as PluginRegistry
participant Hub as PluginHub
participant Unity as UnityEditorPlugin
Client->>Server: HTTP POST /mcp
activate Server
Server->>Middleware: on_call_tool
activate Middleware
Middleware->>Transport: _resolve_user_id_from_request
activate Transport
Transport->>ApiKeySvc: validate(api_key)
activate ApiKeySvc
ApiKeySvc-->>Transport: ValidationResult(valid true user_id)
deactivate ApiKeySvc
Transport-->>Middleware: user_id
deactivate Transport
Middleware->>Middleware: ctx.set_state(user_id)
Middleware->>Middleware: get_active_instance or _maybe_autoselect_instance
Middleware->>Middleware: ctx.set_state(unity_instance)
Middleware-->>Server: call_next()
deactivate Middleware
Server->>Transport: send_with_unity_instance(command_type params unity_instance)
activate Transport
Transport->>Hub: send_command_for_instance(unity_instance command_type params user_id)
activate Hub
Hub->>Registry: _resolve_session_id(unity_instance user_id)
activate Registry
Registry-->>Hub: session_id
deactivate Registry
Hub->>Unity: send_command(session_id command_type params)
Unity-->>Hub: result
Hub-->>Transport: raw_result
deactivate Hub
Transport-->>Server: normalize_unity_response(raw_result)
deactivate Transport
Server-->>Client: MCP tool result
deactivate Server
Sequence diagram for Unity WebSocket plugin authentication and registrationsequenceDiagram
participant Plugin as UnityEditorPlugin
participant Hub as PluginHub
participant ApiKeySvc as ApiKeyService
participant Registry as PluginRegistry
Plugin->>Hub: WS connect /hub/plugin with header X-API-Key
activate Hub
Hub->>ApiKeySvc: validate(api_key)
activate ApiKeySvc
ApiKeySvc-->>Hub: ValidationResult(valid true user_id)
deactivate ApiKeySvc
Hub->>Hub: websocket.state.user_id = user_id
Hub-->>Plugin: accept()
Plugin->>Hub: RegisterMessage(project_name project_hash unity_version)
Hub->>Registry: register(session_id project_name project_hash unity_version user_id)
activate Registry
Registry->>Registry: update _sessions
Registry->>Registry: update _user_hash_to_session[(user_id project_hash)]
Registry-->>Hub: PluginSession
deactivate Registry
Hub-->>Plugin: RegisteredMessage(session_id)
deactivate Hub
Class diagram for API key auth and session isolation componentsclassDiagram
class ApiKeyService {
-str _validation_url
-float _cache_ttl
-str _service_token_header
-str _service_token
-dict~str tuple~bool str dict float~~ _cache
-asyncio.Lock _cache_lock
+REQUEST_TIMEOUT: float
+MAX_RETRIES: int
+ApiKeyService(validation_url: str, cache_ttl: float, service_token_header: str, service_token: str)
+validate(api_key: str) ValidationResult
+invalidate_cache(api_key: str) void
+clear_cache() void
-_validate_external(api_key: str) ValidationResult
+get_instance() ApiKeyService
+is_initialized() bool
}
class ValidationResult {
+bool valid
+str user_id
+dict~str any~ metadata
+str error
+bool cacheable
}
class ServerConfig {
+bool http_remote_hosted
+str api_key_validation_url
+str api_key_login_url
+float api_key_cache_ttl
+str api_key_service_token_header
+str api_key_service_token
}
class PluginSession {
+str session_id
+str project
+str project_hash
+str unity_version
+datetime registered_at
+datetime connected_at
+dict~str ToolDefinitionModel~ tools
+str project_id
+str user_id
}
class PluginRegistry {
-dict~str PluginSession~ _sessions
-dict~str str~ _hash_to_session
-dict~tuple~str str~ str~ _user_hash_to_session
-asyncio.Lock _lock
+register(session_id: str, project_name: str, project_hash: str, unity_version: str, user_id: str) PluginSession
+unregister(session_id: str) void
+get_session(session_id: str) PluginSession
+get_session_id_by_hash(project_hash: str) str
+get_session_id_by_user_hash(user_id: str, project_hash: str) str
+list_sessions(user_id: str) dict~str PluginSession~
+register_tools_for_session(session_id: str, tools: list~ToolDefinitionModel~) void
}
class PluginHub {
<<singleton>>
-dict~str WebSocket~ _connections
+on_connect(websocket: WebSocket) void
+get_sessions(user_id: str) SessionList
+send_command_for_instance(unity_instance: str, command_type: str, params: dict~str any~, user_id: str) dict~str any~
+_resolve_session_id(unity_instance: str, user_id: str) str
}
class UnityInstanceMiddleware {
+get_session_key(ctx: Context) str
+on_call_tool(context: MiddlewareContext, call_next: Callable) any
+on_read_resource(context: MiddlewareContext, call_next: Callable) any
-_inject_unity_instance(context: MiddlewareContext) void
-_maybe_autoselect_instance(ctx: Context) str
-_resolve_user_id() str
}
class UnityTransport {
+send_with_unity_instance(ctx: Context, send_fn: Callable, unity_instance: str, command_type: str, params: dict~str any~, user_id: str) any
+_resolve_user_id_from_request() str
}
ApiKeyService --> ValidationResult
PluginHub --> PluginRegistry
PluginRegistry --> PluginSession
UnityInstanceMiddleware --> PluginHub
UnityInstanceMiddleware --> UnityTransport
UnityInstanceMiddleware --> ServerConfig
UnityTransport --> ApiKeyService
UnityTransport --> ServerConfig
ApiKeyService --> ServerConfig
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (link)
General comments:
- The
X-API-Keyheader name is hard‑coded in multiple places (WebSocket transport, HTTP client config, CLI args) – consider centralizing it in a single constant/shared config to avoid drift if it ever needs to change. - The repeated error message "Unity instance selection is required. Call set_active_instance with Name@hash from mcpforunity://instances." appears in several branches of
_resolve_session_id; factoring this into a small helper (or custom exception) would simplify the control flow and reduce duplication. - ApiKeyService caches entries keyed directly by the full API key string; if you expect many distinct keys or long‑lived servers, consider adding a bounded cache or eviction strategy to avoid unbounded memory growth and to reduce the exposure window of in‑memory secrets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `X-API-Key` header name is hard‑coded in multiple places (WebSocket transport, HTTP client config, CLI args) – consider centralizing it in a single constant/shared config to avoid drift if it ever needs to change.
- The repeated error message "Unity instance selection is required. Call set_active_instance with Name@hash from mcpforunity://instances." appears in several branches of `_resolve_session_id`; factoring this into a small helper (or custom exception) would simplify the control flow and reduce duplication.
- ApiKeyService caches entries keyed directly by the full API key string; if you expect many distinct keys or long‑lived servers, consider adding a bounded cache or eviction strategy to avoid unbounded memory growth and to reduce the exposure window of in‑memory secrets.
## Individual Comments
### Comment 1
<location> `Server/src/main.py:385-394` </location>
<code_context>
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Custom tool execution block is now scoped only to the `no unity_instance` branch, changing behavior and likely breaking targeted instance calls.
With this indentation, `if command_type == "execute_custom_tool":` is only evaluated when *no* `unity_instance` is provided. As a result, `/api/command` calls with `type="execute_custom_tool"` and a specific `unity_instance` will bypass `CustomToolService.execute_tool` and instead fall through to `PluginHub.send_command`, which likely cannot handle this command type. If this change wasn’t intentional, the custom-tool branch should be outdented so it runs after session resolution for both targeted and non-targeted instances, restoring the previous behavior.
</issue_to_address>
### Comment 2
<location> `Server/tests/integration/test_multi_user_session_isolation.py:51-60` </location>
<code_context>
+
+
+class TestBasicValidation:
+ @pytest.mark.asyncio
+ async def test_valid_key(self):
+ svc = _make_service()
</code_context>
<issue_to_address>
**suggestion (testing):** Narrow the expected exception type when resolving another user's session hash
In `test_cannot_resolve_other_users_hash`, `with pytest.raises(Exception):` is too broad and can mask unrelated errors (e.g. typos, unexpected runtime failures). Since `_resolve_session_id` raises a specific exception when no matching session is found, assert that concrete type (e.g. `RuntimeError`) and, if useful, the message, so the test more precisely validates the resolution/authorization behavior.
</issue_to_address>
### Comment 3
<location> `Server/tests/integration/test_api_key_service.py:217` </location>
<code_context>
sk-expiry-key-12345
</code_context>
<issue_to_address>
**security (generic-api-key):** Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@docs/guides/REMOTE_SERVER_AUTH.md`:
- Around line 29-37: The markdown violates MD040/MD060 table/style rules and has
unlabeled code fences and a wording nit: update all tables (including the one
listing CLI args and the other tables around the noted sections) to use
consistent pipe/spacing style to satisfy MD060/MD040, add the language
identifier "http" to all request/headers code fences (e.g., the POST validation
block and the service-token header block) so markdownlint recognizes them, and
replace the phrase "fail closed" with a clearer alternative (e.g., "deny by
default" or "reject by default") wherever it appears (the phrase flagged in the
review). Ensure these same fixes are applied consistently to the other affected
table ranges mentioned in the comment.
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 607-616: RegisterWithCapturedValues currently reads
EditorPrefs.GetString (EditorPrefKeys.ApiKey) inside a method that must be
thread-safe; remove that call and add an apiKey parameter to
RegisterWithCapturedValues and to ConfigureWithCapturedValues so the API key is
captured on the main thread alongside httpUrl/uvxPath etc. Update the callers
(including ConfigureWithCapturedValues) to fetch
EditorPrefs.GetString(EditorPrefKeys.ApiKey) on the main thread and pass the
string down; inside RegisterWithCapturedValues use the passed apiKey when
composing args (instead of calling EditorPrefs) so the method remains
thread-safe.
In
`@MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs`:
- Around line 202-207: EstablishConnectionAsync currently calls
EditorPrefs.GetString(EditorPrefKeys.ApiKey, ...) from a background task
(AttemptReconnectAsync/Task.Run), which is not main-thread safe; instead capture
the API key on the main thread in StartAsync (alongside the other identity
values) into a private field (e.g., _apiKey) and then in
EstablishConnectionAsync use that field when calling
_socket.Options.SetRequestHeader("X-API-Key", ...) rather than calling
EditorPrefs.GetString directly; ensure the field is populated before any
reconnect/attempt logic runs so background threads only read the cached string.
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 842-878: The cachedLoginUrl is not invalidated when the HTTP base
URL changes, causing stale redirects; update the logic so cachedLoginUrl is
cleared or made URL-specific whenever the base URL is updated. Specifically,
either clear cachedLoginUrl inside the method that persists/updates the HTTP URL
(e.g., PersistHttpUrlFromField) after calling HttpEndpointUtility.GetBaseUrl(),
or change GetLoginUrlAsync to store/compare the base URL alongside
cachedLoginUrl (use HttpEndpointUtility.GetBaseUrl() as the key) before
returning the cached value; ensure references to cachedLoginUrl and
GetLoginUrlAsync reflect the new invalidation or key check.
In `@Server/src/main.py`:
- Around line 722-725: The env-parsing for UNITY_MCP_HTTP_REMOTE_HOSTED in the
config assignment (affecting config.http_remote_hosted and reading
args.http_remote_hosted) omits the "on" variant; update the os.environ check to
include "on" alongside "true", "1", and "yes" so that values like "on" enable
remote-hosted mode consistently with other toggles.
In `@Server/src/transport/plugin_hub.py`:
- Around line 80-108: on_connect currently allows connections when ApiKeyService
is not initialized and treats any valid result without user_id as acceptable;
update on_connect to fail closed: if ApiKeyService.is_initialized() is false,
reject the websocket (close with a retryable or auth-required code), and after
calling ApiKeyService.get_instance().validate(api_key) ensure both result.valid
and result.user_id are present before accepting; treat transient auth failures
as retryable by checking result.error for broader conditions (e.g.,
"unavailable", timeouts, 5xx or similar indicators) and close with code 1013 so
clients can retry; only when result.valid and result.user_id exist store
websocket.state.user_id and websocket.state.api_key_metadata.
In `@Server/src/transport/unity_transport.py`:
- Around line 11-12: The API key header lookup is case-sensitive because
get_http_headers() returns a plain dict and the code in unity_transport.py
checks headers.get("x-api-key"); change the logic to normalize header keys to
lowercase before lookup (or perform a lowercase lookup using headers with a
comprehension) so variants like "X-API-Key" are matched reliably; update the
code path around the header extraction in the UnityTransport request handling
(the block that calls get_http_headers() and uses headers.get("x-api-key")) to
use the normalized keys and then pass the value to ApiKeyService validation.
In `@Server/tests/integration/test_api_key_service.py`:
- Around line 212-220: Replace secret-like test literals with clearly non-secret
placeholders to avoid secret-scanner false positives: change occurrences of
"sk-expiry-key-12345" used in svc.validate and in the manual cache-manipulation
block (accessing svc._cache and svc._cache_lock) to a benign test string such as
"test-expiry-key-12345" (or another "test-..." name), and similarly update other
flagged literals (e.g. the "svc-secret-..." patterns at the other mentioned
location) to non-secret placeholders throughout the file so tests retain
semantics but no longer resemble real secrets.
In `@Server/tests/integration/test_middleware_auth_integration.py`:
- Around line 70-84: The test method test_get_session_key_uses_user_id_fallback
in class TestMiddlewareSessionKey declares an unused monkeypatch parameter
causing Ruff ARG002; remove the monkeypatch parameter from the method signature
so it becomes def test_get_session_key_uses_user_id_fallback(self): and run
tests to confirm no other references to monkeypatch in that test need updating.
In `@Server/tests/integration/test_multi_user_session_isolation.py`:
- Around line 95-103: Replace the broad exception assertion in
test_cannot_resolve_other_users_hash: import NoUnitySessionError from
transport.plugin_hub alongside PluginHub, and change the context manager to use
pytest.raises(NoUnitySessionError) when calling
PluginHub._resolve_session_id("hashB1", user_id="userA") so the test asserts the
specific NoUnitySessionError instead of Exception.
In `@Server/tests/integration/test_telemetry_queue_worker.py`:
- Around line 11-16: The test attaches caplog.handler to the telemetry logger
(tel_logger = logging.getLogger("unity-mcp-telemetry")) and sets caplog level,
but never guarantees removal of caplog.handler; wrap the setup and test body in
a try/finally (or convert to a fixture) and in the finally call
tel_logger.removeHandler(caplog.handler) (optionally check if caplog.handler in
tel_logger.handlers) to ensure the handler is always removed and avoid leaking
across tests; keep the caplog.set_level("DEBUG", logger="unity-mcp-telemetry")
setup inside the try so teardown always runs.
🧹 Nitpick comments (6)
Server/tests/integration/test_telemetry_queue_worker.py (1)
27-29: Replace fixed sleep with a condition-based wait to reduce flakiness.
A hard-coded 0.2s delay can still be flaky under slow CI or faster locally. Consider polling for the worker to switch queues (with a bounded timeout) so the test is deterministic.MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
676-685: Consider extracting shared API key header logic.This API key handling block is duplicated from
RegisterWithCapturedValues(lines 607-616). Once the thread-safety issue above is fixed by passing the API key as a parameter, consider extracting a helper method to reduce duplication.Server/src/cli/utils/connection.py (1)
164-180: Consider adding exception chaining for better debugging.The exception handling is thorough, but re-raising without
from eloses the original traceback, which can make debugging harder. This is a minor improvement for maintainability.♻️ Proposed fix to preserve exception chain
except httpx.ConnectError as e: raise UnityConnectionError( f"Cannot connect to Unity MCP server at {cfg.host}:{cfg.port}. " f"Make sure the server is running and Unity is connected.\n" f"Error: {e}" - ) + ) from e except httpx.TimeoutException: raise UnityConnectionError( "Connection to Unity timed out while listing instances. " "Unity may be busy or unresponsive." - ) + ) from None except httpx.HTTPStatusError as e: raise UnityConnectionError( f"HTTP error from server: {e.response.status_code} - {e.response.text}" - ) + ) from e except Exception as e: - raise UnityConnectionError(f"Unexpected error: {e}") + raise UnityConnectionError(f"Unexpected error: {e}") from edocs/reference/REMOTE_SERVER_AUTH_ARCHITECTURE.md (1)
142-150: Consider adding language hints to fenced code blocks.Adding language identifiers (e.g.,
python,text, orplaintext) to fenced code blocks improves syntax highlighting and accessibility in rendered Markdown. The ASCII diagrams at the top are fine without language hints, but the pseudocode sections would benefit.📝 Example fix for one block
-``` +```text _resolve_user_id_from_request() -> if not config.http_remote_hosted: return None ...Also applies to: 189-212, 300-304
Server/src/transport/unity_instance_middleware.py (1)
12-12: Gate user_id-based session keys to remote-hosted mode.
get_session_keyusesuser_idwhenever it is present; if any local-mode code sets it, the "global" fallback is bypassed and sessions may fragment unexpectedly. Consider checkingconfig.http_remote_hostedbefore usinguser_idto match the docstring.♻️ Suggested adjustment
- user_id = ctx.get_state("user_id") - if isinstance(user_id, str) and user_id: - return f"user:{user_id}" + if config.http_remote_hosted: + user_id = ctx.get_state("user_id") + if isinstance(user_id, str) and user_id: + return f"user:{user_id}"Also applies to: 59-75
Server/tests/integration/test_plugin_registry_user_isolation.py (1)
33-48: Make local-mode assumptions explicit in tests.These tests call
list_sessions()withoutuser_id, which only works whenconfig.http_remote_hostedis false. Pinning the mode avoids brittleness if defaults change; apply similarly to other local-mode tests here.🔧 Example adjustment
- async def test_cross_user_isolation_same_hash(self): + async def test_cross_user_isolation_same_hash(self, monkeypatch): + monkeypatch.setattr(config, "http_remote_hosted", False) registry = PluginRegistry()
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs
Show resolved
Hide resolved
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Show resolved
Hide resolved
…sted mode - Validate that user_id is present after successful key validation - Expand transient error detection to include timeout and service errors - Use consistent 1013 status code for retryable auth failures
Consistent with repo
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/src/transport/unity_transport.py (1)
29-69: Fail closed on missing/invalid API key in remote-hosted HTTP path.
Right now, a missing/invalid API key leads touser_id=Noneand a generic retry response from the HTTP path. In remote-hosted mode, that should surface as an auth failure so clients can correct credentials instead of retrying.🔧 Suggested fix (explicit auth failure)
if user_id is None: user_id = await _resolve_user_id_from_request() + + if config.http_remote_hosted and not user_id: + return normalize_unity_response( + MCPResponse( + success=False, + error="auth_required", + message="API key required", + ).model_dump() + )Server/src/transport/plugin_registry.py (1)
49-92: Guard against registering unscoped sessions in remote-hosted mode.
register()currently falls back to local-mode mapping whenuser_idis falsy. If upstream ever missesuser_idin remote-hosted mode, that undermines session isolation. Fail fast whenhttp_remote_hostedis enabled anduser_idis missing.🔧 Suggested fix (fail closed)
async def register( self, session_id: str, project_name: str, project_hash: str, unity_version: str, project_path: str | None = None, user_id: str | None = None, ) -> PluginSession: """Register (or replace) a plugin session. @@ """ + if config.http_remote_hosted and not user_id: + raise ValueError("user_id is required in remote-hosted mode") async with self._lock: now = datetime.now(timezone.utc)
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs`:
- Around line 610-618: The CLI argument string that embeds the API key into args
(the pattern "mcp add --transport http UnityMCP {httpUrl} --header \"X-API-Key:
{apiKey}\"") must shell-escape the apiKey before interpolation to prevent
breaking the command or injection; update the code that constructs args (used in
the async register, sync register, and manual snippet sites) to run the apiKey
through an escaping helper (e.g., escape quotes, backslashes, and problematic
shell chars or percent-encode header values) and use the escaped value in the
header string, or switch to a safe API/parameter array to pass the header
instead of inline string building. Ensure the same escaping helper is applied to
all three call sites where args is built with the X-API-Key header.
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 155-160: The api key is shown in clear text; update the
McpConnectionSection initialization to make the UI field a password field by
setting apiKeyField.isPasswordField = true after you set apiKeyField.value and
tooltip so the input is masked; ensure you still load/save using
EditorPrefKeys.ApiKey and keep existing tooltip behavior when setting
isPasswordField on the apiKeyField VisualElement/TextField instance.
Member
Author
|
@dsarno @Scriptwonder - a million tests later (and new Python tests as well), this remote setup is working well. When hosting the server remotely, there needs to be an external auth provider that can validate the user has access to the MCP server. Two key changes here - the sessions are mapped by session_id and user_id, so a user can only interact with their Unity instances (super important!); everything is gated with an API key. For my own anxiety, I'm going to do some longer tests on local HTTP and stdio and merge this in P.S. Sourcery has a security notice in the PR, but the API key they're referencing is in my test file, and it literally begins with "test" |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/transport/plugin_registry.py (1)
64-94: Avoid mapping selection based onuser_idpresence in local mode.In local mode, a non-
Noneuser_idwould route the session into_user_hash_to_sessionand skip_hash_to_session, breakingget_session_id_by_hashand other local lookups. Consider branching onconfig.http_remote_hostedinstead ofuser_idso local mode always populates_hash_to_session.🔧 Suggested fix
- if user_id: + if config.http_remote_hosted: # Remote-hosted mode: use composite key (user_id, project_hash) composite_key = (user_id, project_hash) previous_session_id = self._user_hash_to_session.get( composite_key) if previous_session_id and previous_session_id != session_id: self._sessions.pop(previous_session_id, None) self._user_hash_to_session[composite_key] = session_id else: # Local mode: use project_hash only previous_session_id = self._hash_to_session.get(project_hash) if previous_session_id and previous_session_id != session_id: self._sessions.pop(previous_session_id, None) self._hash_to_session[project_hash] = session_id
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 155-162: The apiKeyField block uses a non-existent property
passwordCharacter which will not compile; update the TextField usage in
McpConnectionSection (the apiKeyField initialization) to remove
passwordCharacter and instead set apiKeyField.isPasswordField = true and
apiKeyField.maskChar = '*', keeping the existing EditorPrefs value and tooltip
assignments.
🧹 Nitpick comments (1)
Server/src/transport/unity_transport.py (1)
42-43: Consider logging suppressed exceptions for observability.The bare
except Exceptionsilently swallows all errors, which could mask bugs during debugging or production incidents. Since this is a best-effort helper, returningNoneon failure is correct, but logging would help diagnose issues.♻️ Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + async def _resolve_user_id_from_request() -> str | None: """Extract user_id from the current HTTP request's API key header.""" if not config.http_remote_hosted: return None if not ApiKeyService.is_initialized(): return None try: from fastmcp.server.dependencies import get_http_headers headers = get_http_headers(include_all=True) api_key = headers.get("x-api-key") if not api_key: return None service = ApiKeyService.get_instance() result = await service.validate(api_key) return result.user_id if result.valid else None - except Exception: + except Exception as exc: + logger.debug("Failed to resolve user_id from request: %s", exc) return None
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
Show resolved
Hide resolved
Add SanitizeShellHeaderValue() method to escape special shell characters (", \, `, $, !) in API keys before including them in shell command arguments. Apply sanitization to all three locations where API keys are embedded in shell commands (two in RegisterWithCapturedValues, one in GetManualInstructions).
Also fix deprecated passwordCharacter property (now maskChar) and improve exception logging in _resolve_user_id_from_request
…SelectionRequiredError class Add InstanceSelectionRequiredError exception class with centralized error messages (_SELECTION_REQUIRED and _MULTIPLE_INSTANCES). Replace 4 duplicate RuntimeError raises with new exception type. Update tests to catch InstanceSelectionRequiredError instead of RuntimeError.
… constant across C# and Python codebases Add AuthConstants class in C# and API_KEY_HEADER constant in Python to centralize the API key header name definition. Update all 8 locations where "X-API-Key" was hardcoded (4 in C#, 4 in Python) to use the new constants instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Server/src/transport/plugin_hub.py`:
- Around line 282-291: The get_sessions method currently allows global session
listing when user_id is None, which can leak cross-tenant data in remote-hosted
mode; update get_sessions (and the other similar methods referenced) to check
the remote-hosted flag (e.g., a class/global config such as "remote_hosted" or
similar) and if remote-hosted is true and user_id is None, immediately fail
closed by raising a specific exception (or returning an empty/unauthorized
SessionList) instead of calling cls._registry.list_sessions; apply the same
guard to the other session-listing/lookup methods (those around lines 453-558)
that call cls._registry.list_sessions or similar to ensure no unscoped registry
access occurs.
🧹 Nitpick comments (3)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
776-792: Normalize trimmed API key back into the field.
Trimming before persist can leave the UI showing extra whitespace; consider syncing the trimmed value so display matches stored state.♻️ Proposed tweak
string apiKey = apiKeyField.text?.Trim() ?? string.Empty; string existingKey = EditorPrefs.GetString(EditorPrefKeys.ApiKey, string.Empty); if (apiKey != existingKey) { EditorPrefs.SetString(EditorPrefKeys.ApiKey, apiKey); + apiKeyField.SetValueWithoutNotify(apiKey); OnManualConfigUpdateRequested?.Invoke(); McpLog.Info(string.IsNullOrEmpty(apiKey) ? "API key cleared" : "API key updated"); }MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (1)
817-846: Consider escaping newline characters inSanitizeShellHeaderValue.The sanitization method escapes shell metacharacters within double quotes, but doesn't handle newline (
\n) or carriage return (\r) characters. If an API key somehow contained these, it could break the shell command or cause unexpected behavior with the HTTP header.🛡️ Optional hardening to reject control characters
private static string SanitizeShellHeaderValue(string value) { if (string.IsNullOrEmpty(value)) return value; var sb = new System.Text.StringBuilder(value.Length); foreach (char c in value) { switch (c) { case '"': case '\\': case '`': case '$': case '!': sb.Append('\\'); sb.Append(c); break; + case '\n': + case '\r': + case '\0': + // Skip control characters that could break shell/HTTP parsing + break; default: sb.Append(c); break; } } return sb.ToString(); }Server/src/transport/plugin_registry.py (1)
82-95: Clarify behavior whenuser_idis provided in local mode.The branching logic uses
if user_id:to decide between remote-hosted and local mappings. However, if someone accidentally provides auser_idin local mode (whereconfig.http_remote_hostedis false), the session would be stored in_user_hash_to_sessionbutlist_sessions()without auser_idargument would not find it.Consider making the branch explicitly depend on
config.http_remote_hostedfor consistency:🔧 Optional: explicit mode check
- if user_id: + if config.http_remote_hosted and user_id: # Remote-hosted mode: use composite key (user_id, project_hash) composite_key = (user_id, project_hash) ... - else: + elif not config.http_remote_hosted: # Local mode: use project_hash only ... + else: + # This shouldn't happen due to validation at line 65-66 + raise ValueError("user_id is required in remote-hosted mode")
…user session access Remove conditional logic that only filtered sessions by user_id in remote-hosted mode. Now all session listings are filtered by user_id regardless of hosting mode, ensuring users can only see and interact with their own sessions.
…tional user_id parameter Merge get_session_id_by_hash and get_session_id_by_user_hash into a single method that accepts an optional user_id parameter. Update all call sites to use the unified method signature with user_id as the second parameter. Update tests and documentation to reflect the simplified API.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/reference/REMOTE_SERVER_AUTH_ARCHITECTURE.md`:
- Around line 103-107: Table pipe spacing is inconsistent and triggers
markdownlint MD060; normalize each Markdown table (including rows containing
`_hash_to_session` and `_user_hash_to_session`) so pipes align with the header
style by ensuring a single space after and before each cell delimiter,
consistent left/right outer pipes, and the separator row (`|---|---|---|`)
matches column count; apply the same normalization to the other flagged table
blocks (around the ranges you noted) so all tables follow the same pipe/space
alignment.
- Around line 7-65: The fenced diagrams and code blocks in the
REMOTE_SERVER_AUTH_ARCHITECTURE document lack language identifiers (triggering
markdownlint MD040); update each triple-backtick fence around the ASCII diagrams
to use ```text and any actual code snippets to use ```python where
appropriate—specifically adjust the blocks showing
UnityInstanceMiddleware.on_call_tool, _resolve_user_id,
PluginHub.send_command_for_instance, PluginHub.on_connect,
PluginRegistry.register, and the websocket/state examples so each fenced block
includes the correct language tag.
🧹 Nitpick comments (1)
Server/tests/integration/test_plugin_registry_user_isolation.py (1)
9-10: Make local-mode assumptions explicit in this test class.
Several tests assume local mode; consider forcingconfig.http_remote_hosted = Falseby default so environment config can’t leak into these tests.💡 Suggested fixture
class TestRegistryUserIsolation: + `@pytest.fixture`(autouse=True) + def _force_local_mode(self, monkeypatch): + monkeypatch.setattr(config, "http_remote_hosted", False) + `@pytest.mark.asyncio` async def test_register_with_user_id_stores_composite_key(self):
Comment on lines
+7
to
+65
| ``` | ||
| MCP Client MCP Server External Auth | ||
| (Cursor, etc.) (Python) Service | ||
| | | | | ||
| | X-API-Key: abc123 | | | ||
| | POST /mcp (tool call) | | | ||
| |-------------------------->| | | ||
| | | | | ||
| | UnityInstanceMiddleware.on_call_tool | | ||
| | | | | ||
| | _resolve_user_id() | | ||
| | | | | ||
| | | POST /validate | | ||
| | | {"api_key": "abc123"} | | ||
| | |------------------------------>| | ||
| | | | | ||
| | | {"valid":true, | | ||
| | | "user_id":"user-42"} | | ||
| | |<------------------------------| | ||
| | | | | ||
| | Cache result (TTL) | | ||
| | | | | ||
| | ctx.set_state("user_id", "user-42") | | ||
| | ctx.set_state("unity_instance", "Proj@hash") | | ||
| | | | | ||
| | PluginHub.send_command_for_instance | | ||
| | (user_id scoped session lookup) | | ||
| | | | | ||
| | Tool result | | | ||
| |<--------------------------| | | ||
|
|
||
|
|
||
| Unity Plugin MCP Server External Auth | ||
| (C# WebSocket) (Python) Service | ||
| | | | | ||
| | WS /hub/plugin | | | ||
| | X-API-Key: abc123 | | | ||
| |-------------------------->| | | ||
| | | | | ||
| | PluginHub.on_connect | | ||
| | | POST /validate | | ||
| | |------------------------------>| | ||
| | | {"valid":true, ...} | | ||
| | |<------------------------------| | ||
| | | | | ||
| | accept() | | | ||
| | websocket.state.user_id = "user-42" | | ||
| |<--------------------------| | | ||
| | | | | ||
| | {"type":"register", ...} | | | ||
| |-------------------------->| | | ||
| | | | | ||
| | PluginRegistry.register( | | ||
| | ..., user_id="user-42") | | ||
| | _user_hash_to_session[("user-42","hash")] = sid | | ||
| | | | | ||
| | {"type":"registered"} | | | ||
| |<--------------------------| | | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
markdownlint MD040 flags these blocks; use text for diagrams and python for code snippets where appropriate.
Also applies to: 142-150, 189-212, 220-225, 230-235, 300-304, 320-351
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/reference/REMOTE_SERVER_AUTH_ARCHITECTURE.md` around lines 7 - 65, The
fenced diagrams and code blocks in the REMOTE_SERVER_AUTH_ARCHITECTURE document
lack language identifiers (triggering markdownlint MD040); update each
triple-backtick fence around the ASCII diagrams to use ```text and any actual
code snippets to use ```python where appropriate—specifically adjust the blocks
showing UnityInstanceMiddleware.on_call_tool, _resolve_user_id,
PluginHub.send_command_for_instance, PluginHub.on_connect,
PluginRegistry.register, and the websocket/state examples so each fenced block
includes the correct language tag.
Comment on lines
+103
to
+107
| | Index | Key | Used In | | ||
| |-------|-----|---------| | ||
| | `_hash_to_session` | `project_hash -> session_id` | Local mode | | ||
| | `_user_hash_to_session` | `(user_id, project_hash) -> session_id` | Remote-hosted mode | | ||
|
|
Contributor
There was a problem hiding this comment.
Normalize table pipe spacing to satisfy markdownlint.
MD060 flags several tables; align pipes/spaces with the header style.
Also applies to: 259-266, 286-290, 313-317, 356-361
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
105-105: Table column style
Table pipe does not align with header for style "aligned"
(MD060, table-column-style)
105-105: Table column style
Table pipe does not align with header for style "aligned"
(MD060, table-column-style)
105-105: Table column style
Table pipe does not align with header for style "aligned"
(MD060, table-column-style)
106-106: Table column style
Table pipe does not align with header for style "aligned"
(MD060, table-column-style)
106-106: Table column style
Table pipe does not align with header for style "aligned"
(MD060, table-column-style)
106-106: Table column style
Table pipe does not align with header for style "aligned"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@docs/reference/REMOTE_SERVER_AUTH_ARCHITECTURE.md` around lines 103 - 107,
Table pipe spacing is inconsistent and triggers markdownlint MD060; normalize
each Markdown table (including rows containing `_hash_to_session` and
`_user_hash_to_session`) so pipes align with the header style by ensuring a
single space after and before each cell delimiter, consistent left/right outer
pipes, and the separator row (`|---|---|---|`) matches column count; apply the
same normalization to the other flagged table blocks (around the ranges you
noted) so all tables follow the same pipe/space alignment.
Support UNITY_MCP_PROJECT_SCOPED_TOOLS environment variable as alternative to --project-scoped-tools command line flag. Accept "true", "1", "yes", or "on" as truthy values (case-insensitive). Update help text to document the environment variable option.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Server/src/main.py`:
- Around line 758-764: The current check using config.http_remote_hosted alone
can block stdio runs; change the conditional in main.py so the "validation URL
required" error only fires when HTTP transport is selected (e.g., check
config.transport indicates "http") in addition to config.http_remote_hosted and
missing config.api_key_validation_url. Update the if-condition that references
config.http_remote_hosted and config.api_key_validation_url (and the following
logger.error/SystemExit(1) block) to also require config.transport == "http" (or
the equivalent transport enum/value used in your codebase) so non-HTTP
transports are not gated by the API key validation URL.
🧹 Nitpick comments (1)
Server/src/main.py (1)
459-461: Preferlogger.exception(...)to capture stack traces in CLI endpoints.
This keeps diagnostics intact while still returning a safe 500 response.🔧 Suggested change
- except Exception as e: - logger.error(f"CLI command error: {e}") - return JSONResponse({"success": False, "error": str(e)}, status_code=500) + except Exception as e: + logger.exception("CLI command error") + return JSONResponse({"success": False, "error": str(e)}, status_code=500) - except Exception as e: - return JSONResponse({"success": False, "error": str(e)}, status_code=500) + except Exception as e: + logger.exception("CLI instances error") + return JSONResponse({"success": False, "error": str(e)}, status_code=500) - except Exception as e: - logger.error(f"CLI custom tools error: {e}") - return JSONResponse({"success": False, "error": str(e)}, status_code=500) + except Exception as e: + logger.exception("CLI custom tools error") + return JSONResponse({"success": False, "error": str(e)}, status_code=500)Also applies to: 478-479, 540-542
…oth http_remote_hosted is enabled AND transport mode is "http", preventing false validation errors in stdio mode.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Server/src/main.py`:
- Around line 459-461: Replace the logger.error(...) calls inside the exception
handlers that catch "except Exception as e:" (the handler that logs "CLI command
error" and the similar handler around the other JSONResponse return) with
logger.exception(...) so the traceback is preserved; keep the same descriptive
message (e.g., "CLI command error") and still return JSONResponse({"success":
False, "error": str(e)}, status_code=500) in those except blocks so behavior is
unchanged but the full stack trace is logged.
- Around line 406-457: The execute_custom_tool handling is currently nested
inside the "no unity_instance" else branch so calls that include unity_instance
skip custom tool resolution; refactor by pulling the execute_custom_tool block
out so it runs before the final PluginHub.send_command call regardless of
whether unity_instance was supplied — detect command_type ==
"execute_custom_tool", validate params/tool_name/tool_params (as in the current
block), pick session_id/session_details the same way if needed, compute
unity_instance_hint = unity_instance or session_details.hash, call
resolve_project_id_for_unity_instance(unity_instance_hint) and then
CustomToolService.get_instance().execute_tool(project_id, tool_name,
unity_instance_hint, tool_params) and return its result (as JSONResponse) so the
code does not fall through to PluginHub.send_command.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…emote URLs Split HTTP transport into HttpLocal and HttpRemote modes with separate EditorPrefs storage (HttpBaseUrl and HttpRemoteBaseUrl). Add HttpEndpointUtility methods to get/save local and remote URLs independently, and introduce IsRemoteScope() and GetCurrentServerTransport() helpers to centralize 3-way transport determination (Stdio/Http/HttpRemote). Update all client configuration code to distinguish between local and remote HTTP
…remote-hosted mode Update all locations where API key headers are added to HTTP/WebSocket configurations to check HttpEndpointUtility.IsRemoteScope() or serverTransport == HttpRemote before including the API key. This prevents local HTTP mode from unnecessarily including API key headers in shell commands, config JSON, and WebSocket connections.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
294-305:⚠️ Potential issue | 🟠 MajorMove
GetCurrentServerTransport()to the main thread before the async task.
Calling this insideTask.Runrisks touching Unity/EditorPrefs APIs off the main thread. Capture it alongside the other main-thread values.🔧 Proposed fix
- Task.Run(() => - { - try - { - if (client is ClaudeCliMcpConfigurator cliConfigurator) - { - var serverTransport = HttpEndpointUtility.GetCurrentServerTransport(); + var serverTransport = HttpEndpointUtility.GetCurrentServerTransport(); + + Task.Run(() => + { + try + { + if (client is ClaudeCliMcpConfigurator cliConfigurator) + { cliConfigurator.ConfigureWithCapturedValues( projectDir, claudePath, pathPrepend, useHttpTransport, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh, apiKey, serverTransport); } return (success: true, error: (string)null);MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
233-297:⚠️ Potential issue | 🟡 MinorAvoid implicit type coercion for unknown prefs.
Auto-detecting numeric/bool types from string content can silently coerce values like
"00123"or"true"into non-string prefs when a user clicks Save, losing formatting and potentially changing semantics. Consider defaulting unknown keys toStringand letting the user explicitly change the type via the dropdown if needed.💡 Suggested safer default (keep unknown prefs as String)
- if (int.TryParse(stringValue, out var intValue)) - { - item.Type = EditorPrefType.Int; - item.Value = intValue.ToString(); - } - else if (float.TryParse(stringValue, out var floatValue)) - { - item.Type = EditorPrefType.Float; - item.Value = floatValue.ToString(); - } - else if (bool.TryParse(stringValue, out var boolValue)) - { - item.Type = EditorPrefType.Bool; - item.Value = boolValue.ToString(); - } - else - { - item.Type = EditorPrefType.String; - item.Value = stringValue; - } + item.Type = EditorPrefType.String; + item.Value = stringValue;
🤖 Fix all issues with AI agents
In
`@MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs`:
- Around line 531-536: The current mismatch logic marks a transport mismatch
whenever client.ConfiguredTransport differs from the server transport returned
by HttpEndpointUtility.GetCurrentServerTransport(); update the check so that a
mismatch is only set when the serverTransport is known (i.e., not
ConfiguredTransport.Unknown) and the client.ConfiguredTransport is different;
specifically, in the McpClientConfigSection logic that computes
hasTransportMismatch, add a condition to skip setting hasTransportMismatch if
serverTransport == ConfiguredTransport.Unknown (treat Unknown as no mismatch).
🧹 Nitpick comments (1)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
842-878: Consider adding request cancellation support.The
HttpClientusage looks correct with properusingstatement and timeout. However, if the Unity Editor closes or the user navigates away during the request, there's no cancellation mechanism.This is a minor concern since the 10-second timeout provides a reasonable upper bound, and the fire-and-forget nature of the "Get API Key" action is acceptable.
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The MCP HTTP server can be used remotely, but it's not really fit for use remotely.
For e.g. an organization may host this remotely for their company, but anyone with the URL can interact with their server.
Even worse, the fallbacks around mapping Unity sessions with MCP clients sessions can give a user access to a project they should never see. Even the tool to list instances, it shows everyone connected and in a multi-tenant environment that's undesirable.
This change secures remote deployments with a flexible API Key system. MCP allows for Oauth2, and FastMCP supports it as well. But I think approach is better for this server.
In our case, MCP for Unity does not control Unity identities, or any other identities. We can run our own identity provider and take advantage of FastMCP's OAuth feature, but that's out of scope for this server's purpose.
This models allows any auth service to integrate. The external auth services need 2 things:
With just those 2 things, we can fully isolate sessions in the MCP server, so on a remotely hosted machine a user only interacts with their projects. It also stops an unauthorized user from doing invasive things like listing all active instances.
Type of Change
Changes Made
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdI've added documentation for server users and for the dev team.
Related Issues
Closes #433
Additional Notes
Summary by Sourcery
Add API key–based authentication and remote-hosted mode for the MCP for Unity HTTP server, isolating Unity sessions per user and tightening exposed HTTP/CLI surfaces for remote deployments.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.