Skip to content

Commit 664a43b

Browse files
msanatancoderabbitai[bot]
msanatan
and
coderabbitai[bot]
authored
Remote server auth (#644)
* Disable the gloabl default to first session when hosting remotely * Remove calls to /plugin/sessions The newer /api/instances covers that data, and we want to remove these "expose all" endpoints * Disable CLI routes when running in remote hosted mode * Update server README * feat: add API key authentication support for remote-hosted HTTP transport - 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 * feat: add environment variable support for HTTP remote hosted mode - 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 * feat: add user isolation enforcement for remote-hosted mode session listing - 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 * feat: add comprehensive integration tests for API key authentication - 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 * test: add autouse fixture to restore config state after startup validation tests Ensures test isolation for config-dependent integration tests * feat: skip user_id resolution in non-remote-hosted mode Prevents unnecessary API key validation when not in remote-hosted mode * test: add missing mock attributes to instance routing tests - Add client_id to test context mock in set_active_instance test - Add get_state mock to context in global instance routing test * Fix broken telemetry test * Add comprehensive API key authentication documentation - Add user guide covering configuration, setup, and troubleshooting - Add architecture reference documenting internal design and request flows * Add remote-hosted mode and API key authentication documentation to server README * Update reference doc for Docker Hub * Specify exception being caught * Ensure caplog handler cleanup in telemetry queue worker test * Use NoUnitySessionError instead of RuntimeError in session isolation test * Remove unusued monkeypatch arg * Use more obviously fake API keys * Reject connections when ApiKeyService is not initialized in remote-hosted 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 * Accept "on" for UNITY_MCP_HTTP_REMOTE_HOSTED env var Consistent with repo * Invalidate cached login URL when HTTP base URL changes * Pass API key as parameter instead of reading from EditorPrefs in RegisterWithCapturedValues * Cache API key in field instead of reading from EditorPrefs on each reconnection * Align markdown table formatting in remote server auth documentation * Minor fixes * security: Sanitize API key values in shell commands and fix minor issues 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 * Consolidate duplicate instance selection error messages into InstanceSelectionRequiredError 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. * Replace hardcoded "X-API-Key" strings with AuthConstants.ApiKeyHeader 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. * Fix imports * Filter session listing by user_id in all code paths to prevent cross-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. * Consolidate get_session_id_by_hash methods into single method with optional 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. * Add environment variable support for project-scoped-tools flag [skip ci] 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. * Fix Python tests * Update validation logic to only require API key validation URL when both http_remote_hosted is enabled AND transport mode is "http", preventing false validation errors in stdio mode. * Update Server/src/main.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Refactor HTTP transport configuration to support separate local and remote 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 * Only include API key headers in HTTP/WebSocket configuration when in 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. * Hide Manual Server Launch foldout when not in HTTP Local mode * Fix failing test * Improve error messaging and API key validation for HTTP Remote transport Add detailed error messages to WebSocket connection failures that guide users to check server URL, server status, and API key validity. Store error state in TransportState for propagation to UI. Disable "Start Session" button when HTTP Remote mode is selected without an API key, with tooltip explaining requirement. Display error dialog on connection failure with specific error message from transport state. Update connection * Add missing .meta file * Store transport mode in ServerConfig instead of environment variable * Add autouse fixture to restore global config state between tests Add restore_global_config fixture in conftest.py that automatically saves and restores global config attributes and UNITY_MCP_TRANSPORT environment variable between tests. Update integration tests to use monkeypatch.setattr on config.transport_mode instead of monkeypatch.setenv to prevent test pollution and ensure clean state isolation. * Fix startup * Replace _current_transport() calls with direct config.transport_mode access * Minor cleanup * Add integration tests for HTTP transport authentication behavior Verify that HTTP local mode allows requests without user_id while HTTP remote-hosted mode rejects them with auth_required error. * Add smoke tests for transport routing paths across HTTP local, HTTP remote, and stdio modes Verify that HTTP local routes through PluginHub without user_id, HTTP remote routes through PluginHub with user_id, and stdio calls legacy send function with instance_id. Each test uses monkeypatch to configure transport mode and mock appropriate transport layer functions. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 8ee9700 commit 664a43b

48 files changed

Lines changed: 3786 additions & 504 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎MCPForUnity/Editor/Clients/Configurators/CopilotCliConfigurator.cs.meta‎

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs‎

Lines changed: 110 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,17 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true)
156156
}
157157
else if (!string.IsNullOrEmpty(configuredUrl))
158158
{
159-
client.configuredTransport = Models.ConfiguredTransport.Http;
159+
// Distinguish HTTP Local from HTTP Remote by matching against both URLs
160+
string localRpcUrl = HttpEndpointUtility.GetLocalMcpRpcUrl();
161+
string remoteRpcUrl = HttpEndpointUtility.GetRemoteMcpRpcUrl();
162+
if (!string.IsNullOrEmpty(remoteRpcUrl) && UrlsEqual(configuredUrl, remoteRpcUrl))
163+
{
164+
client.configuredTransport = Models.ConfiguredTransport.HttpRemote;
165+
}
166+
else
167+
{
168+
client.configuredTransport = Models.ConfiguredTransport.Http;
169+
}
160170
}
161171
else
162172
{
@@ -173,6 +183,7 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true)
173183
}
174184
else if (!string.IsNullOrEmpty(configuredUrl))
175185
{
186+
// Match against the active scope's URL
176187
string expectedUrl = HttpEndpointUtility.GetMcpRpcUrl();
177188
matches = UrlsEqual(configuredUrl, expectedUrl);
178189
}
@@ -189,9 +200,7 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true)
189200
if (result == "Configured successfully")
190201
{
191202
client.SetStatus(McpStatus.Configured);
192-
// Update transport after rewrite based on current server setting
193-
bool useHttp = EditorConfigurationCache.Instance.UseHttpTransport;
194-
client.configuredTransport = useHttp ? Models.ConfiguredTransport.Http : Models.ConfiguredTransport.Stdio;
203+
client.configuredTransport = HttpEndpointUtility.GetCurrentServerTransport();
195204
}
196205
else
197206
{
@@ -220,9 +229,7 @@ public override void Configure()
220229
if (result == "Configured successfully")
221230
{
222231
client.SetStatus(McpStatus.Configured);
223-
// Set transport based on current server setting
224-
bool useHttp = EditorConfigurationCache.Instance.UseHttpTransport;
225-
client.configuredTransport = useHttp ? Models.ConfiguredTransport.Http : Models.ConfiguredTransport.Stdio;
232+
client.configuredTransport = HttpEndpointUtility.GetCurrentServerTransport();
226233
}
227234
else
228235
{
@@ -272,7 +279,16 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true)
272279
// Determine and set the configured transport type
273280
if (!string.IsNullOrEmpty(url))
274281
{
275-
client.configuredTransport = Models.ConfiguredTransport.Http;
282+
// Distinguish HTTP Local from HTTP Remote
283+
string remoteRpcUrl = HttpEndpointUtility.GetRemoteMcpRpcUrl();
284+
if (!string.IsNullOrEmpty(remoteRpcUrl) && UrlsEqual(url, remoteRpcUrl))
285+
{
286+
client.configuredTransport = Models.ConfiguredTransport.HttpRemote;
287+
}
288+
else
289+
{
290+
client.configuredTransport = Models.ConfiguredTransport.Http;
291+
}
276292
}
277293
else if (args != null && args.Length > 0)
278294
{
@@ -286,6 +302,7 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true)
286302
bool matches = false;
287303
if (!string.IsNullOrEmpty(url))
288304
{
305+
// Match against the active scope's URL
289306
matches = UrlsEqual(url, HttpEndpointUtility.GetMcpRpcUrl());
290307
}
291308
else if (args != null && args.Length > 0)
@@ -313,9 +330,7 @@ public override McpStatus CheckStatus(bool attemptAutoRewrite = true)
313330
if (result == "Configured successfully")
314331
{
315332
client.SetStatus(McpStatus.Configured);
316-
// Update transport after rewrite based on current server setting
317-
bool useHttp = EditorConfigurationCache.Instance.UseHttpTransport;
318-
client.configuredTransport = useHttp ? Models.ConfiguredTransport.Http : Models.ConfiguredTransport.Stdio;
333+
client.configuredTransport = HttpEndpointUtility.GetCurrentServerTransport();
319334
}
320335
else
321336
{
@@ -344,9 +359,7 @@ public override void Configure()
344359
if (result == "Configured successfully")
345360
{
346361
client.SetStatus(McpStatus.Configured);
347-
// Set transport based on current server setting
348-
bool useHttp = EditorConfigurationCache.Instance.UseHttpTransport;
349-
client.configuredTransport = useHttp ? Models.ConfiguredTransport.Http : Models.ConfiguredTransport.Stdio;
362+
client.configuredTransport = HttpEndpointUtility.GetCurrentServerTransport();
350363
}
351364
else
352365
{
@@ -468,9 +481,13 @@ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTran
468481
bool registeredWithStdio = getStdout.Contains("Type: stdio", StringComparison.OrdinalIgnoreCase);
469482

470483
// Set the configured transport based on what we detected
484+
// For HTTP, we can't distinguish local/remote from CLI output alone,
485+
// so infer from the current scope setting when HTTP is detected.
471486
if (registeredWithHttp)
472487
{
473-
client.configuredTransport = Models.ConfiguredTransport.Http;
488+
client.configuredTransport = HttpEndpointUtility.IsRemoteScope()
489+
? Models.ConfiguredTransport.HttpRemote
490+
: Models.ConfiguredTransport.Http;
474491
}
475492
else if (registeredWithStdio)
476493
{
@@ -481,7 +498,7 @@ internal McpStatus CheckStatusWithProjectDir(string projectDir, bool useHttpTran
481498
client.configuredTransport = Models.ConfiguredTransport.Unknown;
482499
}
483500

484-
// Check for transport mismatch
501+
// Check for transport mismatch (3-way: Stdio, Http, HttpRemote)
485502
bool hasTransportMismatch = (currentUseHttp && registeredWithStdio) || (!currentUseHttp && registeredWithHttp);
486503

487504
// For stdio transport, also check package version
@@ -575,7 +592,9 @@ public override void Configure()
575592
public void ConfigureWithCapturedValues(
576593
string projectDir, string claudePath, string pathPrepend,
577594
bool useHttpTransport, string httpUrl,
578-
string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
595+
string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh,
596+
string apiKey,
597+
Models.ConfiguredTransport serverTransport)
579598
{
580599
if (client.status == McpStatus.Configured)
581600
{
@@ -584,7 +603,8 @@ public void ConfigureWithCapturedValues(
584603
else
585604
{
586605
RegisterWithCapturedValues(projectDir, claudePath, pathPrepend,
587-
useHttpTransport, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh);
606+
useHttpTransport, httpUrl, uvxPath, gitUrl, packageName, shouldForceRefresh,
607+
apiKey, serverTransport);
588608
}
589609
}
590610

@@ -594,7 +614,9 @@ public void ConfigureWithCapturedValues(
594614
private void RegisterWithCapturedValues(
595615
string projectDir, string claudePath, string pathPrepend,
596616
bool useHttpTransport, string httpUrl,
597-
string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh)
617+
string uvxPath, string gitUrl, string packageName, bool shouldForceRefresh,
618+
string apiKey,
619+
Models.ConfiguredTransport serverTransport)
598620
{
599621
if (string.IsNullOrEmpty(claudePath))
600622
{
@@ -604,7 +626,16 @@ private void RegisterWithCapturedValues(
604626
string args;
605627
if (useHttpTransport)
606628
{
607-
args = $"mcp add --transport http UnityMCP {httpUrl}";
629+
// Only include API key header for remote-hosted mode
630+
if (serverTransport == Models.ConfiguredTransport.HttpRemote && !string.IsNullOrEmpty(apiKey))
631+
{
632+
string safeKey = SanitizeShellHeaderValue(apiKey);
633+
args = $"mcp add --transport http UnityMCP {httpUrl} --header \"{AuthConstants.ApiKeyHeader}: {safeKey}\"";
634+
}
635+
else
636+
{
637+
args = $"mcp add --transport http UnityMCP {httpUrl}";
638+
}
608639
}
609640
else
610641
{
@@ -626,7 +657,7 @@ private void RegisterWithCapturedValues(
626657

627658
McpLog.Info($"Successfully registered with Claude Code using {(useHttpTransport ? "HTTP" : "stdio")} transport.");
628659
client.SetStatus(McpStatus.Configured);
629-
client.configuredTransport = useHttpTransport ? Models.ConfiguredTransport.Http : Models.ConfiguredTransport.Stdio;
660+
client.configuredTransport = serverTransport;
630661
}
631662

632663
/// <summary>
@@ -664,7 +695,24 @@ private void Register()
664695
if (useHttpTransport)
665696
{
666697
string httpUrl = HttpEndpointUtility.GetMcpRpcUrl();
667-
args = $"mcp add --transport http UnityMCP {httpUrl}";
698+
// Only include API key header for remote-hosted mode
699+
if (HttpEndpointUtility.IsRemoteScope())
700+
{
701+
string apiKey = EditorPrefs.GetString(EditorPrefKeys.ApiKey, string.Empty);
702+
if (!string.IsNullOrEmpty(apiKey))
703+
{
704+
string safeKey = SanitizeShellHeaderValue(apiKey);
705+
args = $"mcp add --transport http UnityMCP {httpUrl} --header \"{AuthConstants.ApiKeyHeader}: {safeKey}\"";
706+
}
707+
else
708+
{
709+
args = $"mcp add --transport http UnityMCP {httpUrl}";
710+
}
711+
}
712+
else
713+
{
714+
args = $"mcp add --transport http UnityMCP {httpUrl}";
715+
}
668716
}
669717
else
670718
{
@@ -715,7 +763,7 @@ private void Register()
715763
// Set status to Configured immediately after successful registration
716764
// The UI will trigger an async verification check separately to avoid blocking
717765
client.SetStatus(McpStatus.Configured);
718-
client.configuredTransport = useHttpTransport ? Models.ConfiguredTransport.Http : Models.ConfiguredTransport.Stdio;
766+
client.configuredTransport = HttpEndpointUtility.GetCurrentServerTransport();
719767
}
720768

721769
private void Unregister()
@@ -757,8 +805,15 @@ public override string GetManualSnippet()
757805
if (useHttpTransport)
758806
{
759807
string httpUrl = HttpEndpointUtility.GetMcpRpcUrl();
808+
// Only include API key header for remote-hosted mode
809+
string headerArg = "";
810+
if (HttpEndpointUtility.IsRemoteScope())
811+
{
812+
string apiKey = EditorPrefs.GetString(EditorPrefKeys.ApiKey, string.Empty);
813+
headerArg = !string.IsNullOrEmpty(apiKey) ? $" --header \"{AuthConstants.ApiKeyHeader}: {SanitizeShellHeaderValue(apiKey)}\"" : "";
814+
}
760815
return "# Register the MCP server with Claude Code:\n" +
761-
$"claude mcp add --transport http UnityMCP {httpUrl}\n\n" +
816+
$"claude mcp add --transport http UnityMCP {httpUrl}{headerArg}\n\n" +
762817
"# Unregister the MCP server:\n" +
763818
"claude mcp remove UnityMCP\n\n" +
764819
"# List registered servers:\n" +
@@ -790,6 +845,37 @@ public override string GetManualSnippet()
790845
"Restart Claude Code"
791846
};
792847

848+
/// <summary>
849+
/// Sanitizes a value for safe inclusion inside a double-quoted shell argument.
850+
/// Escapes characters that are special within double quotes (", \, `, $, !)
851+
/// to prevent shell injection or argument splitting.
852+
/// </summary>
853+
private static string SanitizeShellHeaderValue(string value)
854+
{
855+
if (string.IsNullOrEmpty(value))
856+
return value;
857+
858+
var sb = new System.Text.StringBuilder(value.Length);
859+
foreach (char c in value)
860+
{
861+
switch (c)
862+
{
863+
case '"':
864+
case '\\':
865+
case '`':
866+
case '$':
867+
case '!':
868+
sb.Append('\\');
869+
sb.Append(c);
870+
break;
871+
default:
872+
sb.Append(c);
873+
break;
874+
}
875+
}
876+
return sb.ToString();
877+
}
878+
793879
/// <summary>
794880
/// Extracts the package source (--from argument value) from claude mcp get output.
795881
/// The output format includes args like: --from "mcpforunityserver==9.0.1"

‎MCPForUnity/Editor/Constants/AuthConstants.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
namespace MCPForUnity.Editor.Constants
2+
{
3+
/// <summary>
4+
/// Protocol-level constants for API key authentication.
5+
/// </summary>
6+
internal static class AuthConstants
7+
{
8+
internal const string ApiKeyHeader = "X-API-Key";
9+
}
10+
}

‎MCPForUnity/Editor/Constants/AuthConstants.cs.meta‎

Lines changed: 11 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎MCPForUnity/Editor/Constants/EditorPrefKeys.cs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ internal static class EditorPrefKeys
2424
internal const string ClaudeCliPathOverride = "MCPForUnity.ClaudeCliPath";
2525

2626
internal const string HttpBaseUrl = "MCPForUnity.HttpUrl";
27+
internal const string HttpRemoteBaseUrl = "MCPForUnity.HttpRemoteUrl";
2728
internal const string SessionId = "MCPForUnity.SessionId";
2829
internal const string WebSocketUrlOverride = "MCPForUnity.WebSocketUrl";
2930
internal const string GitUrlOverride = "MCPForUnity.GitUrlOverride";
@@ -55,5 +56,7 @@ internal static class EditorPrefKeys
5556

5657
internal const string TelemetryDisabled = "MCPForUnity.TelemetryDisabled";
5758
internal const string CustomerUuid = "MCPForUnity.CustomerUUID";
59+
60+
internal const string ApiKey = "MCPForUnity.ApiKey";
5861
}
5962
}

‎MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,26 @@ private static void PopulateUnityNode(JObject unity, string uvPath, McpClient cl
7171
if (unity["command"] != null) unity.Remove("command");
7272
if (unity["args"] != null) unity.Remove("args");
7373

74+
// Only include API key header for remote-hosted mode
75+
if (HttpEndpointUtility.IsRemoteScope())
76+
{
77+
string apiKey = EditorPrefs.GetString(EditorPrefKeys.ApiKey, string.Empty);
78+
if (!string.IsNullOrEmpty(apiKey))
79+
{
80+
var headers = new JObject { [AuthConstants.ApiKeyHeader] = apiKey };
81+
unity["headers"] = headers;
82+
}
83+
else
84+
{
85+
if (unity["headers"] != null) unity.Remove("headers");
86+
}
87+
}
88+
else
89+
{
90+
// Local HTTP doesn't use API keys; remove any stale headers
91+
if (unity["headers"] != null) unity.Remove("headers");
92+
}
93+
7494
if (isVSCode)
7595
{
7696
unity["type"] = "http";

0 commit comments

Comments
 (0)