Add LAN HTTP transport mode#1073
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds explicit LAN-scoped HTTP transport support: new transport enum, persisted LAN public/bind URLs, LAN-aware endpoint helpers and launch validation, transport selection changes across configurators, server management, auto-start, shutdown, and connection UI with a LAN warning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
MCPForUnity/Editor/Services/Server/ServerCommandBuilder.cs (1)
33-44:⚠️ Potential issue | 🟡 MinorDefault error message hardcodes "HTTP Local launch" even in LAN scope.
When
launchUrlAllowedis false andlocalUrlErroris empty, the fallback message incorrectly says "not allowed for HTTP Local launch" even when the user is in LAN scope. The LAN validator sets an error in all failure paths today, so the fallback rarely triggers, but it's still misleading. Consider branching the message or renaminglocalUrlError→policyErrorand making the fallback scope-aware.✏️ Proposed fix
- string httpUrl = HttpEndpointUtility.GetLocalServerLaunchBaseUrl(); - string localUrlError; - bool launchUrlAllowed = HttpEndpointUtility.IsLanScope() - ? HttpEndpointUtility.IsHttpLanUrlAllowedForLaunch(httpUrl, out localUrlError) - : HttpEndpointUtility.IsHttpLocalUrlAllowedForLaunch(httpUrl, out localUrlError); + bool isLanScope = HttpEndpointUtility.IsLanScope(); + string httpUrl = isLanScope + ? HttpEndpointUtility.GetLanBindBaseUrl() + : HttpEndpointUtility.GetLocalBaseUrl(); + string policyError; + bool launchUrlAllowed = isLanScope + ? HttpEndpointUtility.IsHttpLanUrlAllowedForLaunch(httpUrl, out policyError) + : HttpEndpointUtility.IsHttpLocalUrlAllowedForLaunch(httpUrl, out policyError); if (!launchUrlAllowed) { - error = string.IsNullOrEmpty(localUrlError) - ? $"The configured URL ({httpUrl}) is not allowed for HTTP Local launch." - : $"{localUrlError} (configured URL: {httpUrl})"; + string launchMode = isLanScope ? "HTTP LAN" : "HTTP Local"; + error = string.IsNullOrEmpty(policyError) + ? $"The configured URL ({httpUrl}) is not allowed for {launchMode} launch." + : $"{policyError} (configured URL: {httpUrl})"; return false; }Capturing
isLanScopelocally also avoids the double read ofIsLanScope()(once insideGetLocalServerLaunchBaseUrland once on line 35), which removes a small possibility of URL/validator mismatch if the scope pref changes between reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/Server/ServerCommandBuilder.cs` around lines 33 - 44, The fallback error message in the ServerCommandBuilder logic hardcodes "HTTP Local launch" even when in LAN scope; capture the scope once (bool isLanScope = HttpEndpointUtility.IsLanScope()) before calling GetLocalServerLaunchBaseUrl/validator to avoid double reads, rename localUrlError → policyError (or similar) for clarity, and when launchUrlAllowed is false set error using a scope-aware fallback: if policyError is empty use different messages for LAN vs Local (e.g., "not allowed for HTTP LAN launch" vs "not allowed for HTTP Local launch"), otherwise append the configured URL to policyError as currently done; update references to IsHttpLanUrlAllowedForLaunch and IsHttpLocalUrlAllowedForLaunch accordingly.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
634-638:⚠️ Potential issue | 🟡 MinorTooltip fallback mislabels LAN mode as "HTTP Local".
httpLocalSelectedhere isIsHttpLocalServerSelected()which is true for bothHTTPLocalandHTTPLan. WhencanStartLocalServeris false andlocalUrlErrorhappens to be null, the fallback message says "HTTP Local requires a loopback URL (localhost/127.0.0.1/::1)." — that's wrong guidance for LAN (LAN actually requires0.0.0.0/::). Same issue exists for the error-dialog fallback strings at Lines 438, 683, and 838 which all say "HTTP Local URL is blocked…" even when LAN is the current selection.💡 Proposed fix for the tooltip fallback
startHttpServerButton.tooltip = httpLocalSelected ? (canStartLocalServer ? string.Empty - : localUrlError ?? $"HTTP Local requires a loopback URL ({HttpEndpointUtility.GetHttpLocalHostRequirementText()}).") + : localUrlError ?? (IsHttpLanSelected() + ? "LAN HTTP requires a bind URL such as http://0.0.0.0:<port>." + : $"HTTP Local requires a loopback URL ({HttpEndpointUtility.GetHttpLocalHostRequirementText()}).")) : string.Empty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs` around lines 634 - 638, The tooltip fallback and several error-dialog fallback strings use IsHttpLocalServerSelected() (httpLocalSelected) which is true for both HTTPLocal and HTTPLan, so change the conditional to detect LAN explicitly (use IsHttpLanServerSelected() or equivalent) and choose the correct message accordingly: when LAN is selected, use HttpEndpointUtility.GetHttpLanRequirementText() and label messages "HTTP LAN" / advise 0.0.0.0/::; when Local is selected use HttpEndpointUtility.GetHttpLocalHostRequirementText() and label "HTTP Local" / advise loopback addresses; update the startHttpServerButton.tooltip expression (the localUrlError/canStartLocalServer branch) and the corresponding error-dialog fallback strings that currently say "HTTP Local URL is blocked…" so they conditionally pick the LAN vs Local wording and requirement-text helper functions.MCPForUnity/Editor/Services/ServerManagementService.cs (2)
379-462:⚠️ Potential issue | 🟡 MinorScope-switch while server is running can desync reachability/running checks.
IsLocalHttpServerRunning()andIsLocalHttpServerReachable()both re-derive the port viaGetLocalServerLaunchBaseUrl(), which branches onIsLanScope()at call time. If a user starts a server on one scope (e.g., LAN port 8090) and then switches the transport to a different scope (e.g., HTTP Local port 8080), these methods will probe the new scope's port — miss the still-running original listener — and incorrectly report "no server" while an orphan server keeps running.Although the launch port is already cached in two places (EditorPrefs via
StoreTracking()and encoded in the pidfile path itself viaTryGetPortFromPidFilePath()), the handshake-based detection logic (lines 397–406) still queriesGetListeningProcessIdsForPort(port)using the current-scope port rather than the original launch port. This undermines the detection when scope has changed.Consider using the original cached launch port (extractable from the pidfile path or
StoreTracking()) when checking running status and reachability, rather than the current-scope-derived port.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/ServerManagementService.cs` around lines 379 - 462, Both checks derive the port from GetLocalServerLaunchBaseUrl() which can change with IsLanScope(), causing desync; update IsLocalHttpServerRunning() and IsLocalHttpServerReachable() to prefer the original launch port when a handshake/pidfile exists: call TryGetLocalHttpServerHandshake(out pidFilePath, out token) and then use TryGetPortFromPidFilePath(pidFilePath, out var launchPort) (falling back to the current uri.Port only if that fails). In IsLocalHttpServerRunning(), use launchPort when calling GetListeningProcessIdsForPort(launchPort) and when checking TryGetStoredLocalServerPid(launchPort,...); in IsLocalHttpServerReachable(), if a launchPort was obtained use TryConnectToLocalPort(host, launchPort, timeoutMs) instead of the scope-derived port. Keep existing fallbacks and error handling; reference functions: IsLocalHttpServerRunning, IsLocalHttpServerReachable, TryGetLocalHttpServerHandshake, TryGetPortFromPidFilePath, TryReadPidFromPidFile, GetListeningProcessIdsForPort, TryGetStoredLocalServerPid, TryConnectToLocalPort, and StoreTracking.
899-923:⚠️ Potential issue | 🟡 Minor
IsLocalUrl()now recognizes bind-all addresses (0.0.0.0, ::) as "local".Because
GetLocalServerLaunchBaseUrl()returnshttp://0.0.0.0:<port>in LAN scope, andIsLocalUrl()now treats bind-all addresses as "local" viaIsBindAllInterfacesHost(), LAN URLs will be classified as local. This affects back-compat code inMcpConnectionSection.cs(line 139) which infers scope as "local" vs "remote" when no preference is set—a LAN-scoped bind-all URL will now be classified as "local" instead of "remote."The behavior is documented in test assertions (
ServerManagementServiceCharacterizationTests.csline 124–134). The interface documentation (IServerManagementService.IsLocalUrl()) should be updated to clarify that bind-all addresses are included in the "local" classification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/ServerManagementService.cs` around lines 899 - 923, The public IsLocalUrl() behavior was broadened to treat bind-all addresses (0.0.0.0/::) as local because GetLocalServerLaunchBaseUrl() can return http://0.0.0.0:<port> and the private IsLocalUrl(string) uses HttpEndpointUtility.IsBindAllInterfacesHost(host); update the IServerManagementService.IsLocalUrl() interface documentation to explicitly state that bind-all interfaces (0.0.0.0 and ::) are considered "local" by IsLocalUrl(), and add a brief note in McpConnectionSection (where scope is inferred) documenting that LAN-scoped bind-all URLs will now be classified as local so callers can adjust preference logic if needed.
🧹 Nitpick comments (5)
MCPForUnity/Editor/Services/HttpAutoStartHandler.cs (1)
76-85: Consider extracting the scope-aware derive+validate pattern into a helper.The exact same "pick URL by scope, then pick validator by scope" pattern is now in both
ServerCommandBuilder.TryBuildCommandand here. A single helper such asHttpEndpointUtility.TryGetLocalLaunchUrl(out string url, out string error)would centralize the scope branching, eliminate the double read ofIsLanScope()within each call site, and keep the two sites from drifting (e.g., if LAN gains additional policy checks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/HttpAutoStartHandler.cs` around lines 76 - 85, Extract the scope-aware "derive URL then validate by scope" logic into a new helper on HttpEndpointUtility (e.g., TryGetLocalLaunchUrl(out string launchBaseUrl, out string policyError) returning bool) and replace the duplicated blocks in HttpAutoStartHandler and ServerCommandBuilder.TryBuildCommand to call this helper; implement the helper to call GetLocalServerLaunchBaseUrl() once, check IsLanScope(), and then call either IsHttpLanUrlAllowedForLaunch(...) or IsHttpLocalUrlAllowedForLaunch(...) to populate policyError and return the allow boolean so callers simply receive launchBaseUrl and policyError and can early-return on false.MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs (1)
51-55: Consider renaminghttpLocalSelectednow that LAN is included.The variable name no longer reflects its semantics accurately since LAN is now grouped in. A name like
httpLocalOrLanSelectedorshouldStopLocalServerwould improve clarity. The guard comment on line 13 ("If HTTP Local is selected…") is similarly outdated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs` around lines 51 - 55, Rename the misleading variable httpLocalSelected in McpEditorShutdownCleanup (and update its guard comment) to reflect that it includes LAN—e.g., httpLocalOrLanSelected or shouldStopLocalServer; update all references where httpLocalSelected is used (the boolean expression built from useHttp, scope, and MCPServiceLocator.Server.IsLocalUrl()) and change the explanatory comment ("If HTTP Local is selected…") to match the new name/semantics so the code and comment accurately indicate that both local and LAN scopes are considered.MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs (2)
26-26: Hardcoded192.168.1.10default may not match the user's LAN.The default public URL points at a specific private IP that won't apply to most users. Users will realize it only when a client can't connect. Consider either:
- Leaving the field empty by default (like
DefaultRemoteBaseUrl) so the UI prompts the user, or- Auto-detecting a LAN IP (e.g., first non-loopback, non-virtual IPv4) on first use and persisting that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs` at line 26, DefaultLanPublicBaseUrl is hardcoded to a single private IP that won't work for most users; change its initialization to a safe default and/or auto-detect a LAN IP. Replace the const DefaultLanPublicBaseUrl = "http://192.168.1.10:8090" with either an empty string (same approach as DefaultRemoteBaseUrl) so the UI prompts for input, or implement an auto-detect helper (e.g., TryGetLocalLanIp or GetPreferredLanIp) that scans System.Net.NetworkInformation.NetworkInterface/UnicastIPAddressInformation for the first non-loopback, non-virtual IPv4, constructs the base URL with the intended port, and use that detected value on first-run (persist it in the same store you use elsewhere). Ensure you update any code that references DefaultLanPublicBaseUrl to handle an empty value and fall back to prompting the user.
340-344: LAN mode hardcoded tohttp://.LAN will reject
https://bind URLs outright. That's consistent with the PR's "trusted private network" framing, but it means self-signed HTTPS setups (reverse proxy on the same host, etc.) aren't supported. Worth documenting in the README or the LAN warning banner if not already.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs` around lines 340 - 344, LAN mode currently rejects https:// in HttpEndpointUtility by checking uri.Scheme (the current check in the validation branch around uri.Scheme.Equals("http", StringComparison.OrdinalIgnoreCase)); update the validation to either accept https or — if you want to keep the current restriction — change the error message to be explicit and add documentation: modify the error string produced when uri.Scheme is not "http" to explain that LAN mode requires plain HTTP for trusted private-network operation and why HTTPS (e.g., self-signed certs or reverse-proxy setups) is rejected, and add a short note to the README (or the LAN warning banner code path) describing this limitation and recommended workaround (use a reverse proxy with proper certificates or disable LAN mode); locate and update the URL validation in HttpEndpointUtility and the README/banner text accordingly.MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
570-586: Mix of UI-dropdown and cache-derived scope across helpers.
IsHttpLanSelected()readstransportDropdown.value, whileGetLocalServerLaunchBaseUrl()internally readsHttpEndpointUtility.IsLanScope()(fromEditorConfigurationCache). These are kept in sync in the normal callback flow, but using two sources of truth for the same logical decision insideTryGetLocalHttpLaunchPolicyis fragile if the initialization order ever changes. Recommend delegating toHttpEndpointUtility.IsLanScope()for parity withGetLocalServerLaunchBaseUrl().♻️ Proposed refactor
private bool TryGetLocalHttpLaunchPolicy(out string localBaseUrl, out string localUrlError) { localBaseUrl = HttpEndpointUtility.GetLocalServerLaunchBaseUrl(); - return IsHttpLanSelected() + return HttpEndpointUtility.IsLanScope() ? HttpEndpointUtility.IsHttpLanUrlAllowedForLaunch(localBaseUrl, out localUrlError) : HttpEndpointUtility.IsHttpLocalUrlAllowedForLaunch(localBaseUrl, out localUrlError); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs` around lines 570 - 586, The helpers mix dropdown state and cached scope causing two sources of truth; update the code so all scope checks use the cache-based API instead of the UI control: change IsHttpLanSelected (or the call site in TryGetLocalHttpLaunchPolicy) to delegate to HttpEndpointUtility.IsLanScope() (the same source used by HttpEndpointUtility.GetLocalServerLaunchBaseUrl()), and remove direct reads of transportDropdown.value for this decision so TryGetLocalHttpLaunchPolicy always uses the cache-driven IsLanScope() path when choosing between IsHttpLanUrlAllowedForLaunch and IsHttpLocalUrlAllowedForLaunch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MCPForUnity/Editor/Clients/Configurators/OpenClawConfigurator.cs`:
- Around line 390-393: ServerMatchesCurrentEndpoint incorrectly rejects the LAN
RPC URL; update ServerMatchesCurrentEndpoint so that when ResolveTransport or
configuredTransport is HttpLan it accepts HttpEndpointUtility.GetLanMcpRpcUrl()
in the same way it accepts GetLocalMcpRpcUrl() and GetRemoteMcpRpcUrl().
Specifically, modify the else-branch URL equality check inside
ServerMatchesCurrentEndpoint (the block that compares configuredUrl to
HttpEndpointUtility.GetLocalMcpRpcUrl()/GetRemoteMcpRpcUrl()) to also compare
against HttpEndpointUtility.GetLanMcpRpcUrl(), ensuring that a configured
transport of ConfiguredTransport.HttpLan will return true and prevent
CheckStatus from misreporting IncorrectPath or causing auto-rewrite loops.
In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs`:
- Around line 103-112: In SaveLanPublicBaseUrl, the current check uses uri.Port
> 0 which treats a missing port as 80 and can overwrite the default bind port;
change the derivation to only set LanBindPrefKey when the parsed Uri has an
explicit port by checking !uri.IsDefaultPort (or explicitly validate/require a
port from NormalizeBaseUrl before saving), and ensure this aligns with
IsHttpLanUrlAllowedForLaunch’s policy (which should continue to reject uri.Port
<= 0 if you choose to reject missing ports upstream); update
SaveLanPublicBaseUrl to use !uri.IsDefaultPort when deciding to set
LanBindPrefKey (keeping LanPublicPrefKey behavior unchanged).
- Around line 325-359: The IsHttpLanUrlAllowedForLaunch method incorrectly uses
uri.Port <= 0 to detect missing explicit ports; replace that dead check with
Uri.IsDefaultPort (e.g., if (uri.IsDefaultPort) { error = "LAN HTTP bind URL
requires an explicit port."; return false; }) and then (optionally) validate
uri.Port is within the valid TCP port range if you want extra safety before
proceeding to IsBindAllInterfacesHost; update references to uri.Port only after
the IsDefaultPort check.
---
Outside diff comments:
In `@MCPForUnity/Editor/Services/Server/ServerCommandBuilder.cs`:
- Around line 33-44: The fallback error message in the ServerCommandBuilder
logic hardcodes "HTTP Local launch" even when in LAN scope; capture the scope
once (bool isLanScope = HttpEndpointUtility.IsLanScope()) before calling
GetLocalServerLaunchBaseUrl/validator to avoid double reads, rename
localUrlError → policyError (or similar) for clarity, and when launchUrlAllowed
is false set error using a scope-aware fallback: if policyError is empty use
different messages for LAN vs Local (e.g., "not allowed for HTTP LAN launch" vs
"not allowed for HTTP Local launch"), otherwise append the configured URL to
policyError as currently done; update references to IsHttpLanUrlAllowedForLaunch
and IsHttpLocalUrlAllowedForLaunch accordingly.
In `@MCPForUnity/Editor/Services/ServerManagementService.cs`:
- Around line 379-462: Both checks derive the port from
GetLocalServerLaunchBaseUrl() which can change with IsLanScope(), causing
desync; update IsLocalHttpServerRunning() and IsLocalHttpServerReachable() to
prefer the original launch port when a handshake/pidfile exists: call
TryGetLocalHttpServerHandshake(out pidFilePath, out token) and then use
TryGetPortFromPidFilePath(pidFilePath, out var launchPort) (falling back to the
current uri.Port only if that fails). In IsLocalHttpServerRunning(), use
launchPort when calling GetListeningProcessIdsForPort(launchPort) and when
checking TryGetStoredLocalServerPid(launchPort,...); in
IsLocalHttpServerReachable(), if a launchPort was obtained use
TryConnectToLocalPort(host, launchPort, timeoutMs) instead of the scope-derived
port. Keep existing fallbacks and error handling; reference functions:
IsLocalHttpServerRunning, IsLocalHttpServerReachable,
TryGetLocalHttpServerHandshake, TryGetPortFromPidFilePath,
TryReadPidFromPidFile, GetListeningProcessIdsForPort,
TryGetStoredLocalServerPid, TryConnectToLocalPort, and StoreTracking.
- Around line 899-923: The public IsLocalUrl() behavior was broadened to treat
bind-all addresses (0.0.0.0/::) as local because GetLocalServerLaunchBaseUrl()
can return http://0.0.0.0:<port> and the private IsLocalUrl(string) uses
HttpEndpointUtility.IsBindAllInterfacesHost(host); update the
IServerManagementService.IsLocalUrl() interface documentation to explicitly
state that bind-all interfaces (0.0.0.0 and ::) are considered "local" by
IsLocalUrl(), and add a brief note in McpConnectionSection (where scope is
inferred) documenting that LAN-scoped bind-all URLs will now be classified as
local so callers can adjust preference logic if needed.
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 634-638: The tooltip fallback and several error-dialog fallback
strings use IsHttpLocalServerSelected() (httpLocalSelected) which is true for
both HTTPLocal and HTTPLan, so change the conditional to detect LAN explicitly
(use IsHttpLanServerSelected() or equivalent) and choose the correct message
accordingly: when LAN is selected, use
HttpEndpointUtility.GetHttpLanRequirementText() and label messages "HTTP LAN" /
advise 0.0.0.0/::; when Local is selected use
HttpEndpointUtility.GetHttpLocalHostRequirementText() and label "HTTP Local" /
advise loopback addresses; update the startHttpServerButton.tooltip expression
(the localUrlError/canStartLocalServer branch) and the corresponding
error-dialog fallback strings that currently say "HTTP Local URL is blocked…" so
they conditionally pick the LAN vs Local wording and requirement-text helper
functions.
---
Nitpick comments:
In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs`:
- Line 26: DefaultLanPublicBaseUrl is hardcoded to a single private IP that
won't work for most users; change its initialization to a safe default and/or
auto-detect a LAN IP. Replace the const DefaultLanPublicBaseUrl =
"http://192.168.1.10:8090" with either an empty string (same approach as
DefaultRemoteBaseUrl) so the UI prompts for input, or implement an auto-detect
helper (e.g., TryGetLocalLanIp or GetPreferredLanIp) that scans
System.Net.NetworkInformation.NetworkInterface/UnicastIPAddressInformation for
the first non-loopback, non-virtual IPv4, constructs the base URL with the
intended port, and use that detected value on first-run (persist it in the same
store you use elsewhere). Ensure you update any code that references
DefaultLanPublicBaseUrl to handle an empty value and fall back to prompting the
user.
- Around line 340-344: LAN mode currently rejects https:// in
HttpEndpointUtility by checking uri.Scheme (the current check in the validation
branch around uri.Scheme.Equals("http", StringComparison.OrdinalIgnoreCase));
update the validation to either accept https or — if you want to keep the
current restriction — change the error message to be explicit and add
documentation: modify the error string produced when uri.Scheme is not "http" to
explain that LAN mode requires plain HTTP for trusted private-network operation
and why HTTPS (e.g., self-signed certs or reverse-proxy setups) is rejected, and
add a short note to the README (or the LAN warning banner code path) describing
this limitation and recommended workaround (use a reverse proxy with proper
certificates or disable LAN mode); locate and update the URL validation in
HttpEndpointUtility and the README/banner text accordingly.
In `@MCPForUnity/Editor/Services/HttpAutoStartHandler.cs`:
- Around line 76-85: Extract the scope-aware "derive URL then validate by scope"
logic into a new helper on HttpEndpointUtility (e.g., TryGetLocalLaunchUrl(out
string launchBaseUrl, out string policyError) returning bool) and replace the
duplicated blocks in HttpAutoStartHandler and
ServerCommandBuilder.TryBuildCommand to call this helper; implement the helper
to call GetLocalServerLaunchBaseUrl() once, check IsLanScope(), and then call
either IsHttpLanUrlAllowedForLaunch(...) or IsHttpLocalUrlAllowedForLaunch(...)
to populate policyError and return the allow boolean so callers simply receive
launchBaseUrl and policyError and can early-return on false.
In `@MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs`:
- Around line 51-55: Rename the misleading variable httpLocalSelected in
McpEditorShutdownCleanup (and update its guard comment) to reflect that it
includes LAN—e.g., httpLocalOrLanSelected or shouldStopLocalServer; update all
references where httpLocalSelected is used (the boolean expression built from
useHttp, scope, and MCPServiceLocator.Server.IsLocalUrl()) and change the
explanatory comment ("If HTTP Local is selected…") to match the new
name/semantics so the code and comment accurately indicate that both local and
LAN scopes are considered.
In `@MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs`:
- Around line 570-586: The helpers mix dropdown state and cached scope causing
two sources of truth; update the code so all scope checks use the cache-based
API instead of the UI control: change IsHttpLanSelected (or the call site in
TryGetLocalHttpLaunchPolicy) to delegate to HttpEndpointUtility.IsLanScope()
(the same source used by HttpEndpointUtility.GetLocalServerLaunchBaseUrl()), and
remove direct reads of transportDropdown.value for this decision so
TryGetLocalHttpLaunchPolicy always uses the cache-driven IsLanScope() path when
choosing between IsHttpLanUrlAllowedForLaunch and
IsHttpLocalUrlAllowedForLaunch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db91d8d5-23ae-4cc6-8134-88b3567e24b4
📒 Files selected for processing (11)
MCPForUnity/Editor/Clients/Configurators/OpenClawConfigurator.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Helpers/HttpEndpointUtility.csMCPForUnity/Editor/Models/McpStatus.csMCPForUnity/Editor/Services/HttpAutoStartHandler.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.csMCPForUnity/Editor/Services/Server/ServerCommandBuilder.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.uxml
| if (UrlsEqual(configuredUrl, HttpEndpointUtility.GetLanMcpRpcUrl())) | ||
| { | ||
| return ConfiguredTransport.HttpLan; | ||
| } |
There was a problem hiding this comment.
ServerMatchesCurrentEndpoint doesn't accept the LAN URL and will treat correctly configured LAN servers as mismatched.
ResolveTransport now returns HttpLan for the LAN RPC URL, but the URL validation in ServerMatchesCurrentEndpoint (lines 354–360) only accepts GetLocalMcpRpcUrl() or GetRemoteMcpRpcUrl(). When the user has selected LAN scope and the OpenClaw config contains the LAN URL, configuredTransport == expectedTransport == HttpLan, but the else-branch URL check rejects the LAN URL and returns false, causing CheckStatus to either trigger an auto-rewrite loop or report IncorrectPath.
🛠️ Proposed fix
string configuredUrl = server["url"]?.ToString();
if (string.IsNullOrWhiteSpace(configuredUrl) ||
(!UrlsEqual(configuredUrl, HttpEndpointUtility.GetLocalMcpRpcUrl()) &&
- !UrlsEqual(configuredUrl, HttpEndpointUtility.GetRemoteMcpRpcUrl())))
+ !UrlsEqual(configuredUrl, HttpEndpointUtility.GetRemoteMcpRpcUrl()) &&
+ !UrlsEqual(configuredUrl, HttpEndpointUtility.GetLanMcpRpcUrl())))
{
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Clients/Configurators/OpenClawConfigurator.cs` around
lines 390 - 393, ServerMatchesCurrentEndpoint incorrectly rejects the LAN RPC
URL; update ServerMatchesCurrentEndpoint so that when ResolveTransport or
configuredTransport is HttpLan it accepts HttpEndpointUtility.GetLanMcpRpcUrl()
in the same way it accepts GetLocalMcpRpcUrl() and GetRemoteMcpRpcUrl().
Specifically, modify the else-branch URL equality check inside
ServerMatchesCurrentEndpoint (the block that compares configuredUrl to
HttpEndpointUtility.GetLocalMcpRpcUrl()/GetRemoteMcpRpcUrl()) to also compare
against HttpEndpointUtility.GetLanMcpRpcUrl(), ensuring that a configured
transport of ConfiguredTransport.HttpLan will return true and prevent
CheckStatus from misreporting IncorrectPath or causing auto-rewrite loops.
| public static void SaveLanPublicBaseUrl(string userValue) | ||
| { | ||
| string normalized = NormalizeBaseUrl(userValue, DefaultLanPublicBaseUrl, remoteScope: false); | ||
| EditorPrefs.SetString(LanPublicPrefKey, normalized); | ||
|
|
||
| if (Uri.TryCreate(normalized, UriKind.Absolute, out var uri) && uri.Port > 0) | ||
| { | ||
| EditorPrefs.SetString(LanBindPrefKey, $"http://0.0.0.0:{uri.Port}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -name "HttpEndpointUtility.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 115
🏁 Script executed:
# Once we find the file, let's examine the relevant section
cat -n MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs | sed -n '95,120p'Repository: CoplayDev/unity-mcp
Length of output: 1400
🏁 Script executed:
# Let's also search for the IsHttpLanUrlAllowedForLaunch method mentioned in the comment
rg "IsHttpLanUrlAllowedForLaunch" -A 10 -B 2Repository: CoplayDev/unity-mcp
Length of output: 6817
🏁 Script executed:
# Search for references to DefaultLanPublicBaseUrl and LanBindPrefKey to understand the defaults
rg "DefaultLanPublicBaseUrl|LanBindPrefKey|LanPublicPrefKey" -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 3145
🏁 Script executed:
# Get the full IsHttpLanUrlAllowedForLaunch method
rg "public static bool IsHttpLanUrlAllowedForLaunch" -A 30Repository: CoplayDev/unity-mcp
Length of output: 2587
🏁 Script executed:
# Search for any other Uri.Port usages in the file to check for similar issues
rg "uri\.Port" MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs -B 3 -A 3Repository: CoplayDev/unity-mcp
Length of output: 644
🏁 Script executed:
# Let's check if Uri.IsDefaultPort is used anywhere
rg "IsDefaultPort" MCPForUnity/Editor/Helpers/HttpEndpointUtility.csRepository: CoplayDev/unity-mcp
Length of output: 45
LAN bind URL can silently collapse to port 80.
When the user saves a public URL without an explicit port (e.g., http://192.168.1.10), Uri.Port returns 80 (the HTTP default), not -1 or 0. The guard on line 108 passes, silently setting the bind URL to http://0.0.0.0:80 and overwriting the default http://0.0.0.0:8090 without warning. The policy check in IsHttpLanUrlAllowedForLaunch won't catch this either, since it only rejects uri.Port <= 0.
Gate the derivation on !uri.IsDefaultPort, or reject/ask for an explicit port upstream in SaveBaseUrl.
🛠️ Proposed fix
- if (Uri.TryCreate(normalized, UriKind.Absolute, out var uri) && uri.Port > 0)
+ // Only sync the bind URL when the user supplied an explicit port.
+ // Uri.Port falls back to the scheme default (80 for http), so `> 0` alone would
+ // silently rewrite the bind URL to port 80 when the user omitted the port.
+ if (Uri.TryCreate(normalized, UriKind.Absolute, out var uri) && !uri.IsDefaultPort && uri.Port > 0)
{
EditorPrefs.SetString(LanBindPrefKey, $"http://0.0.0.0:{uri.Port}");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void SaveLanPublicBaseUrl(string userValue) | |
| { | |
| string normalized = NormalizeBaseUrl(userValue, DefaultLanPublicBaseUrl, remoteScope: false); | |
| EditorPrefs.SetString(LanPublicPrefKey, normalized); | |
| if (Uri.TryCreate(normalized, UriKind.Absolute, out var uri) && uri.Port > 0) | |
| { | |
| EditorPrefs.SetString(LanBindPrefKey, $"http://0.0.0.0:{uri.Port}"); | |
| } | |
| } | |
| public static void SaveLanPublicBaseUrl(string userValue) | |
| { | |
| string normalized = NormalizeBaseUrl(userValue, DefaultLanPublicBaseUrl, remoteScope: false); | |
| EditorPrefs.SetString(LanPublicPrefKey, normalized); | |
| // Only sync the bind URL when the user supplied an explicit port. | |
| // Uri.Port falls back to the scheme default (80 for http), so `> 0` alone would | |
| // silently rewrite the bind URL to port 80 when the user omitted the port. | |
| if (Uri.TryCreate(normalized, UriKind.Absolute, out var uri) && !uri.IsDefaultPort && uri.Port > 0) | |
| { | |
| EditorPrefs.SetString(LanBindPrefKey, $"http://0.0.0.0:{uri.Port}"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs` around lines 103 - 112, In
SaveLanPublicBaseUrl, the current check uses uri.Port > 0 which treats a missing
port as 80 and can overwrite the default bind port; change the derivation to
only set LanBindPrefKey when the parsed Uri has an explicit port by checking
!uri.IsDefaultPort (or explicitly validate/require a port from NormalizeBaseUrl
before saving), and ensure this aligns with IsHttpLanUrlAllowedForLaunch’s
policy (which should continue to reject uri.Port <= 0 if you choose to reject
missing ports upstream); update SaveLanPublicBaseUrl to use !uri.IsDefaultPort
when deciding to set LanBindPrefKey (keeping LanPublicPrefKey behavior
unchanged).
| public static bool IsHttpLanUrlAllowedForLaunch(string url, out string error) | ||
| { | ||
| error = null; | ||
| if (string.IsNullOrWhiteSpace(url)) | ||
| { | ||
| error = "LAN HTTP requires a bind URL such as http://0.0.0.0:8090."; | ||
| return false; | ||
| } | ||
|
|
||
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | ||
| { | ||
| error = $"Invalid LAN HTTP bind URL: {url}"; | ||
| return false; | ||
| } | ||
|
|
||
| if (!uri.Scheme.Equals("http", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| error = "LAN HTTP bind URL must use http://."; | ||
| return false; | ||
| } | ||
|
|
||
| if (uri.Port <= 0) | ||
| { | ||
| error = "LAN HTTP bind URL requires an explicit port."; | ||
| return false; | ||
| } | ||
|
|
||
| if (IsBindAllInterfacesHost(uri.Host)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| error = "LAN HTTP server bind host must be 0.0.0.0 or ::."; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What does System.Uri.Port return when a URL like "http://0.0.0.0" has no explicit port?
💡 Result:
System.Uri.Port returns 80 for the URL "http://0.0.0.0" when no explicit port is specified.
Citations:
- 1: https://msdn.microsoft.com/library/system.uri.port
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.uri.port?view=net-10.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.uri.port?view=net-9.0
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.uri.isdefaultport?view=net-10.0
🏁 Script executed:
cat -n MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs | sed -n '325,359p'Repository: CoplayDev/unity-mcp
Length of output: 1391
🌐 Web query:
System.Uri.IsDefaultPort property behavior in .NET
💡 Result:
The System.Uri.IsDefaultPort property in .NET returns true if the port of the URI matches the default port defined for its scheme (e.g., 80 for http, 443 for https), and false otherwise. It is only valid for absolute URIs; accessing it on relative URIs throws InvalidOperationException. Key behaviors: - If no port is specified in the URI string, it uses the scheme's default, so IsDefaultPort is true (e.g., new Uri("http://example.com") has Port=80 and IsDefaultPort=true). - If a non-default port is specified, IsDefaultPort is false (e.g., new Uri("http://example.com:8080") has Port=8080 and IsDefaultPort=false). - If the default port is explicitly specified, Uri normalizes by removing it from AbsoluteUri, but Port still returns the default value and IsDefaultPort remains true (e.g., new Uri("http://example.com:80") normalizes to "http://example.com/", Port=80, IsDefaultPort=true). - For schemes without a defined default port, Port returns -1 and IsDefaultPort is true unless a port was explicitly parsed. - The property checks internal flags (NotAny(Flags.NotDefaultPort)) set during parsing against the UriParser's DefaultPort for the scheme. Examples: Uri uri1 = new Uri("http://www.contoso.com/"); // IsDefaultPort = true (implicit port 80) Uri uri2 = new Uri("http://www.contoso.com:8080/"); // IsDefaultPort = false Uri uri3 = new Uri("https://www.contoso.com:443/"); // IsDefaultPort = true (explicit default) Uri uri4 = new Uri("https://www.contoso.com:80/"); // IsDefaultPort = false This behavior is consistent across .NET Framework, .NET Core, and .NET 5+ (up to .NET 10.0 docs).
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.uri.isdefaultport?view=net-10.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.uri.isdefaultport?view=net-9.0
- 3: https://msdn.microsoft.com/ja-jp/library/system.uri.isdefaultport
- 4: https://stackoverflow.com/questions/28047632/how-is-request-url-isdefaultport-determined
- 5: https://www.includehelp.com/dot-net/uri-isdefaultport-property-with-example.aspx
- 6: https://learn.microsoft.com/en-us/dotnet/api/system.uri?view=net-10.0
- 7: https://github.com/dotnet/dotnet/blob/b0f34d51fccc69fd334253924abd8d6853fad7aa/src/runtime/src/libraries/System.Private.Uri/src/System/Uri.cs
- 8: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Uri/docs/System.Uri%20Threat%20Model.md
- 9: https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-uri
uri.Port <= 0 check is dead code — Uri.Port returns the scheme default (80) when no port is specified.
Per .NET documentation, if no port is specified in a URI, Uri.Port returns the default for the scheme. For http URLs, that default is 80, not -1 or 0. So a URL like http://0.0.0.0 (without explicit port) will:
- Pass
Uri.TryCreate✓ - Pass the scheme check ✓
- Pass the
uri.Port <= 0guard withuri.Port == 80— the "requires an explicit port" error never fires - Pass the bind-all host check → validation returns true
The server then attempts to bind to port 80, which requires elevated permissions and fails opaquely at runtime. Use uri.IsDefaultPort to properly detect when a port was not explicitly provided:
🛡️ Proposed fix using `IsDefaultPort`
- if (uri.Port <= 0)
+ // Uri.Port returns the scheme default (80) when unspecified, so use IsDefaultPort
+ // to require the user to supply an explicit port for LAN bind URLs.
+ if (uri.IsDefaultPort)
{
error = "LAN HTTP bind URL requires an explicit port.";
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static bool IsHttpLanUrlAllowedForLaunch(string url, out string error) | |
| { | |
| error = null; | |
| if (string.IsNullOrWhiteSpace(url)) | |
| { | |
| error = "LAN HTTP requires a bind URL such as http://0.0.0.0:8090."; | |
| return false; | |
| } | |
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | |
| { | |
| error = $"Invalid LAN HTTP bind URL: {url}"; | |
| return false; | |
| } | |
| if (!uri.Scheme.Equals("http", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| error = "LAN HTTP bind URL must use http://."; | |
| return false; | |
| } | |
| if (uri.Port <= 0) | |
| { | |
| error = "LAN HTTP bind URL requires an explicit port."; | |
| return false; | |
| } | |
| if (IsBindAllInterfacesHost(uri.Host)) | |
| { | |
| return true; | |
| } | |
| error = "LAN HTTP server bind host must be 0.0.0.0 or ::."; | |
| return false; | |
| } | |
| public static bool IsHttpLanUrlAllowedForLaunch(string url, out string error) | |
| { | |
| error = null; | |
| if (string.IsNullOrWhiteSpace(url)) | |
| { | |
| error = "LAN HTTP requires a bind URL such as http://0.0.0.0:8090."; | |
| return false; | |
| } | |
| if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) | |
| { | |
| error = $"Invalid LAN HTTP bind URL: {url}"; | |
| return false; | |
| } | |
| if (!uri.Scheme.Equals("http", StringComparison.OrdinalIgnoreCase)) | |
| { | |
| error = "LAN HTTP bind URL must use http://."; | |
| return false; | |
| } | |
| // Uri.Port returns the scheme default (80) when unspecified, so use IsDefaultPort | |
| // to require the user to supply an explicit port for LAN bind URLs. | |
| if (uri.IsDefaultPort) | |
| { | |
| error = "LAN HTTP bind URL requires an explicit port."; | |
| return false; | |
| } | |
| if (IsBindAllInterfacesHost(uri.Host)) | |
| { | |
| return true; | |
| } | |
| error = "LAN HTTP server bind host must be 0.0.0.0 or ::."; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Helpers/HttpEndpointUtility.cs` around lines 325 - 359,
The IsHttpLanUrlAllowedForLaunch method incorrectly uses uri.Port <= 0 to detect
missing explicit ports; replace that dead check with Uri.IsDefaultPort (e.g., if
(uri.IsDefaultPort) { error = "LAN HTTP bind URL requires an explicit port.";
return false; }) and then (optionally) validate uri.Port is within the valid TCP
port range if you want extra safety before proceeding to
IsBindAllInterfacesHost; update references to uri.Port only after the
IsDefaultPort check.
1 participant
Description
Type of Change
Save your change type
Changes Made
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes / Improvements