From c90f63da66add7200acde3df8249da4d09dcbd79 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:41:04 -0400 Subject: [PATCH 1/7] ci: make compile-check.sh work from a Windows checkout A core.autocrlf checkout gives tools/compile-defines.txt and tools/compile-refs/*.txt CRLF endings; the read loops kept the CR, so every -define: carried a stray \r and every LIBCACHE/ reference failed to resolve, failing the Editor build on TestRunner types. Strip the CR when reading and pin those manifests to LF in .gitattributes. Also normalise REPO/UNITY_DATA/OUT through pwd -W so Git Bash's /x/... paths do not reach Roslyn as X:\x/..., and document the Windows recipe in the header. --- .gitattributes | 5 +++++ CLAUDE.md | 4 ++++ tools/compile-check.sh | 29 ++++++++++++++++++++++------- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/.gitattributes b/.gitattributes index 2698dc2fa..81ecf83b3 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1,3 +1,8 @@ # Shell scripts must keep LF: tools/compile-check.sh runs inside a Linux container, # where CRLF would fail with "bad interpreter". *.sh text eol=lf + +# compile-check.sh reads these line by line; a CR from a core.autocrlf checkout would +# become part of every -define: symbol and reference name. +tools/compile-defines.txt text eol=lf +tools/compile-refs/*.txt text eol=lf diff --git a/CLAUDE.md b/CLAUDE.md index 5ebb8a182..549a927a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -165,6 +165,10 @@ cd Server && uv run pytest tests/ -k "test_create_material" -v # Local multi-version compile check (parity with CI matrix, see tools/unity-versions.json) tools/check-unity-versions.sh # compile-only across installed Unity Hub editors tools/check-unity-versions.sh --full # full EditMode test run + +# License-free Roslyn compile of MCPForUnity, the same gate compile-check.yml runs on every PR. +# No Editor launch, ~1 min per version; the script header has the Windows/Git Bash recipe. +UNITY_DATA=/path/to/Editor/Data UNITY_VERSION=2021.3.45f2 tools/compile-check.sh ``` #### Local headless test harness diff --git a/tools/compile-check.sh b/tools/compile-check.sh index 4f4fd7658..8d7331077 100644 --- a/tools/compile-check.sh +++ b/tools/compile-check.sh @@ -13,6 +13,13 @@ # Usage (inside unityci/editor, or against a local Hub install): # UNITY_DATA=/opt/unity/Editor/Data UNITY_VERSION=2021.3.45f2 tools/compile-check.sh # +# Windows, from Git Bash against a Hub install (no Docker, no license): +# UNITY_DATA="C:/Program Files/Unity/Hub/Editor/2021.3.45f2/Editor/Data" UNITY_VERSION=2021.3.45f2 \ +# EXTRA_REFS=/c/refs tools/compile-check.sh +# where /c/refs holds Newtonsoft.Json.dll and nunit.framework.dll, e.g. copied from +# TestProjects/UnityMCPTests/Library/PackageCache/com.unity.nuget.newtonsoft-json@*/Runtime/ and +# .../com.unity.ext.nunit@*/net35/unity-custom/. Takes ~1 min per Unity version. +# # Env: # UNITY_DATA Editor/Data directory (default /opt/unity/Editor/Data) # UNITY_VERSION e.g. 2021.3.45f2 (required for version defines) @@ -29,16 +36,20 @@ # open TestProjects/UnityMCPTests in that Editor, then re-derive from the generated csprojs. set -uo pipefail -UNITY_DATA=${UNITY_DATA:-/opt/unity/Editor/Data} -REPO=${REPO:-"$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"} +die() { echo "::error::$*" >&2; exit 2; } + +# Roslyn runs as a Windows process under Git Bash, so every path handed to it must be +# X:/... form, not the MSYS /x/... form that `pwd` produces there. `pwd -W` is the MSYS +# spelling; on Linux/macOS it is an invalid option and we fall back to plain `pwd`. +winpath() { (cd "$1" 2>/dev/null && (pwd -W 2>/dev/null || pwd)) || die "directory not found: $1"; } + +UNITY_DATA=$(winpath "${UNITY_DATA:-/opt/unity/Editor/Data}") +REPO=$(winpath "${REPO:-"$(dirname "${BASH_SOURCE[0]}")/.."}") EXTRA_REFS=${EXTRA_REFS:-"$REPO/.compile-refs"} PLATFORMS=${PLATFORMS:-"win osx linux"} OUT=${OUT:-/tmp/mcp-compile-check} +mkdir -p "$OUT" && OUT=$(winpath "$OUT") LIBCACHE="$UNITY_DATA/Resources/PackageManager/ProjectTemplates/libcache" - -die() { echo "::error::$*" >&2; exit 2; } - -[ -d "$UNITY_DATA" ] || die "UNITY_DATA not found: $UNITY_DATA" CSC="$UNITY_DATA/DotNetSdkRoslyn/csc.dll" [ -f "$CSC" ] || die "Roslyn compiler not found: $CSC" @@ -111,10 +122,14 @@ compile() { echo "-preferreduilang:en-US" echo "-nowarn:CS1701,CS1702" # benign netstandard facade version unification echo "-out:$dir/$name.dll" - while read -r d; do [ -n "$d" ] && echo "-define:$d"; done < "$REPO/tools/compile-defines.txt" + # ${var%$'\r'} strips the CR a core.autocrlf checkout appends to every line: a CR inside + # -define:FOO silently defines the wrong symbol, and inside a LIBCACHE/ name it makes + # `find -name` match nothing, so the Editor build fails on TestRunner/UI types. + while read -r d; do d=${d%$'\r'}; [ -n "$d" ] && echo "-define:$d"; done < "$REPO/tools/compile-defines.txt" version_defines | while read -r d; do echo "-define:$d"; done platform_defines "$platform" | while read -r d; do echo "-define:$d"; done while read -r entry; do + entry=${entry%$'\r'} [ -n "$entry" ] || continue local p; p=$(resolve_ref "$entry") if [ -n "$p" ] && [ -f "$p" ]; then echo "-r:\"$p\""; nrefs=$((nrefs+1)) From df0cba78e64973a778b3a8fceec57d02976ec662 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:41:05 -0400 Subject: [PATCH 2/7] chore: keep Server/uv.lock in step with the version bump The lock's own-project entry still said 10.1.0 while pyproject.toml said 10.2.0, so uv sync --locked failed and CI silently re-resolved. update_versions.py now rewrites the mcpforunityserver entry, release.yml stages the lock in the bump commit, and python-tests.yml runs uv sync --locked --extra dev so pytest uses the pinned dev dependencies instead of whatever uv pip install fetches on the day. --- .github/workflows/python-tests.yml | 7 +- .github/workflows/release.yml | 2 +- Server/uv.lock | 2 +- tools/tests/test_update_versions.py | 124 ++++++++++++++++++++++++++++ tools/update_versions.py | 52 ++++++++++++ 5 files changed, 183 insertions(+), 4 deletions(-) create mode 100644 tools/tests/test_update_versions.py diff --git a/.github/workflows/python-tests.yml b/.github/workflows/python-tests.yml index d21112b5c..0f4315ba4 100644 --- a/.github/workflows/python-tests.yml +++ b/.github/workflows/python-tests.yml @@ -47,8 +47,11 @@ jobs: - name: Install dependencies run: | cd Server - uv sync - uv pip install -e ".[dev]" + # --locked fails if uv.lock disagrees with pyproject.toml instead of silently + # re-resolving, so a dependency or version bump that forgot `uv lock` shows up here. + # The dev extra is resolved in the lock too, so pytest runs against pinned versions + # instead of whatever `uv pip install` would fetch today. + uv sync --locked --extra dev - name: Run tests with coverage run: | diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4815e7ca7..a7e7aa4e0 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -227,7 +227,7 @@ jobs: git config user.name "GitHub Actions" git config user.email "actions@github.com" git checkout -b "$BRANCH" - git add MCPForUnity/package.json manifest.json "Server/pyproject.toml" Server/README.md + git add MCPForUnity/package.json manifest.json "Server/pyproject.toml" Server/uv.lock Server/README.md if git diff --cached --quiet; then echo "No version changes to commit." else diff --git a/Server/uv.lock b/Server/uv.lock index 81a005bf9..4074402da 100644 --- a/Server/uv.lock +++ b/Server/uv.lock @@ -858,7 +858,7 @@ wheels = [ [[package]] name = "mcpforunityserver" -version = "10.1.0" +version = "10.2.0" source = { editable = "." } dependencies = [ { name = "click" }, diff --git a/tools/tests/test_update_versions.py b/tools/tests/test_update_versions.py new file mode 100644 index 000000000..26988c4b8 --- /dev/null +++ b/tools/tests/test_update_versions.py @@ -0,0 +1,124 @@ +"""Tests for tools/update_versions.py. + +The release bump used to leave Server/uv.lock behind: pyproject.toml moved to 10.2.0 +while the lock still recorded 10.1.0, which `uv sync --locked` rejects. These tests pin +the lock updater to the lock format uv actually writes for this repo, so a format change +surfaces here rather than in a release run. +""" +import re +import shutil +import sys +from pathlib import Path + +import pytest + +_TOOLS_DIR = Path(__file__).resolve().parents[1] +if str(_TOOLS_DIR) not in sys.path: + sys.path.insert(0, str(_TOOLS_DIR)) + +import update_versions # noqa: E402 + +REAL_LOCK = _TOOLS_DIR.parent / "Server" / "uv.lock" +REAL_PYPROJECT = _TOOLS_DIR.parent / "Server" / "pyproject.toml" + +SAMPLE_LOCK = ( + 'version = 1\n' + 'revision = 3\n' + 'requires-python = ">=3.10"\n' + '\n' + '[[package]]\n' + 'name = "click"\n' + 'version = "8.3.1"\n' + 'source = { registry = "https://pypi.org/simple" }\n' + '\n' + '[[package]]\n' + 'name = "mcpforunityserver"\n' + 'version = "10.1.0"\n' + 'source = { editable = "." }\n' + 'dependencies = [\n' + ' { name = "click" },\n' + ']\n' + '\n' + '[[package]]\n' + 'name = "mcp"\n' + 'version = "1.26.0"\n' + 'source = { registry = "https://pypi.org/simple" }\n' +) + + +@pytest.fixture +def lock_file(tmp_path, monkeypatch): + path = tmp_path / "uv.lock" + path.write_bytes(SAMPLE_LOCK.encode("utf-8")) + monkeypatch.setattr(update_versions, "UV_LOCK", path) + return path + + +def test_update_uv_lock_rewrites_only_the_project_entry(lock_file): + assert update_versions.update_uv_lock("10.2.0") is True + updated = lock_file.read_bytes().decode("utf-8") + assert 'name = "mcpforunityserver"\nversion = "10.2.0"' in updated + assert 'name = "click"\nversion = "8.3.1"' in updated + assert 'name = "mcp"\nversion = "1.26.0"' in updated + assert updated.count('version = "10.2.0"') == 1 + + +def test_update_uv_lock_is_a_noop_when_already_current(lock_file): + update_versions.update_uv_lock("10.2.0") + before = lock_file.read_bytes() + assert update_versions.update_uv_lock("10.2.0") is False + assert lock_file.read_bytes() == before + + +def test_update_uv_lock_dry_run_does_not_write(lock_file): + assert update_versions.update_uv_lock("10.2.0", dry_run=True) is True + assert 'version = "10.1.0"' in lock_file.read_bytes().decode("utf-8") + + +def test_update_uv_lock_preserves_crlf_line_endings(tmp_path, monkeypatch): + """A core.autocrlf checkout must not be rewritten to LF (or vice versa) by a version bump.""" + path = tmp_path / "uv.lock" + path.write_bytes(SAMPLE_LOCK.replace("\n", "\r\n").encode("utf-8")) + monkeypatch.setattr(update_versions, "UV_LOCK", path) + assert update_versions.update_uv_lock("10.2.0") is True + raw = path.read_bytes() + assert b"\r\n" in raw + assert b"\n" not in raw.replace(b"\r\n", b"") + assert b'name = "mcpforunityserver"\r\nversion = "10.2.0"' in raw + + +def test_update_uv_lock_missing_entry_is_reported_not_raised(tmp_path, monkeypatch): + path = tmp_path / "uv.lock" + path.write_bytes(b'version = 1\n\n[[package]]\nname = "click"\nversion = "8.3.1"\n') + monkeypatch.setattr(update_versions, "UV_LOCK", path) + assert update_versions.update_uv_lock("10.2.0") is False + assert b'version = "8.3.1"' in path.read_bytes() + + +def test_update_uv_lock_missing_file_is_reported_not_raised(tmp_path, monkeypatch): + monkeypatch.setattr(update_versions, "UV_LOCK", tmp_path / "absent.lock") + assert update_versions.update_uv_lock("10.2.0") is False + + +def test_update_uv_lock_matches_the_checked_in_lock_format(tmp_path, monkeypatch): + """The regex has to keep matching whatever uv writes for this repo.""" + path = tmp_path / "uv.lock" + shutil.copy(REAL_LOCK, path) + monkeypatch.setattr(update_versions, "UV_LOCK", path) + assert update_versions.update_uv_lock("0.0.0.dev0") is True + assert re.search( + r'^\[\[package\]\]\s*\nname = "mcpforunityserver"\s*\nversion = "0\.0\.0\.dev0"', + path.read_bytes().decode("utf-8"), + re.MULTILINE, + ) + + +def test_checked_in_lock_agrees_with_pyproject_version(): + """Guards the drift that `uv sync --locked` now rejects in CI.""" + pyproject_version = re.search( + r'^version = "([^"]+)"', REAL_PYPROJECT.read_text(encoding="utf-8"), re.MULTILINE + ).group(1) + lock_version = update_versions._UV_LOCK_SELF_VERSION.search( + REAL_LOCK.read_bytes().decode("utf-8") + ).group(2) + assert lock_version == pyproject_version diff --git a/tools/update_versions.py b/tools/update_versions.py index 979be4ec5..a232d8936 100755 --- a/tools/update_versions.py +++ b/tools/update_versions.py @@ -5,6 +5,7 @@ - MCPForUnity/package.json (Unity package version) - manifest.json (MCP bundle manifest) - Server/pyproject.toml (Python package version) +- Server/uv.lock (the project's own entry, so `uv sync --locked` keeps passing) - Server/README.md (version references) - README.md (fixed version examples) - docs/i18n/README-zh.md (fixed version examples) @@ -37,6 +38,7 @@ PACKAGE_JSON = REPO_ROOT / "MCPForUnity" / "package.json" MANIFEST_JSON = REPO_ROOT / "manifest.json" PYPROJECT_TOML = REPO_ROOT / "Server" / "pyproject.toml" +UV_LOCK = REPO_ROOT / "Server" / "uv.lock" SERVER_README = REPO_ROOT / "Server" / "README.md" ROOT_README = REPO_ROOT / "README.md" ZH_README = REPO_ROOT / "docs" / "i18n" / "README-zh.md" @@ -141,6 +143,53 @@ def update_pyproject_toml(new_version: str, dry_run: bool = False) -> bool: return True +# uv.lock records the project's own version next to its resolved dependencies. Bumping +# pyproject.toml without this line makes `uv sync --locked` fail and left the lock stale +# for whole release cycles (10.1.0 in the lock while pyproject said 10.2.0). The block is +# the only place the project's version appears, so a targeted rewrite is equivalent to +# `uv lock` here without requiring uv on the release runner. +_UV_LOCK_SELF_VERSION = re.compile( + r'(^\[\[package\]\]\s*\nname = "mcpforunityserver"\s*\nversion = ")([^"]+)(")', + re.MULTILINE, +) + + +def _display(path: Path) -> str: + """Repo-relative for the normal case; absolute when tests point at a temp file.""" + try: + return str(path.relative_to(REPO_ROOT)) + except ValueError: + return str(path) + + +def update_uv_lock(new_version: str, dry_run: bool = False) -> bool: + """Update the mcpforunityserver entry in Server/uv.lock.""" + if not UV_LOCK.exists(): + print(f"Warning: {_display(UV_LOCK)} not found") + return False + + # Bytes round-trip: text mode would rewrite the file's line endings to os.linesep. + content = UV_LOCK.read_bytes().decode("utf-8") + match = _UV_LOCK_SELF_VERSION.search(content) + if not match: + print(f"Warning: Could not find the mcpforunityserver entry in {_display(UV_LOCK)}") + return False + + current_version = match.group(2) + if current_version == new_version: + print(f"✓ {_display(UV_LOCK)} already at v{new_version}") + return False + + print(f"Updating {_display(UV_LOCK)}: {current_version} → {new_version}") + + if not dry_run: + content = _UV_LOCK_SELF_VERSION.sub( + lambda m: f"{m.group(1)}{new_version}{m.group(3)}", content, count=1) + UV_LOCK.write_bytes(content.encode("utf-8")) + + return True + + def update_server_readme(new_version: str, dry_run: bool = False) -> bool: """Update version references in Server/README.md.""" if not SERVER_README.exists(): @@ -262,6 +311,9 @@ def main() -> int: if update_pyproject_toml(version, args.dry_run): updates_made.append("Server/pyproject.toml") + if update_uv_lock(version, args.dry_run): + updates_made.append("Server/uv.lock") + if update_server_readme(version, args.dry_run): updates_made.append("Server/README.md") From 5ad752e7e2b730ae128c40fcc42562cb25162245 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:41:05 -0400 Subject: [PATCH 3/7] fix(editor): pin stdio for the CI boot session without rewriting EditorPrefs McpCiBoot wrote UseHttpTransport=false to the developer's EditorPrefs and still lost to the value EditorConfigurationCache had already read: on a machine whose prefs choose HTTP with auto-start, UNITY_MCP_ALLOW_BATCH let HttpAutoStartHandler run and BridgeControlService.StartAsync stopped the stdio bridge the harness was talking to. Keep the override in SessionState so it survives domain reloads, dies with the editor process, and never touches the user's real preference. --- MCPForUnity/Editor/McpCiBoot.cs | 13 +++-- .../Services/EditorConfigurationCache.cs | 37 +++++++++++- .../Services/EditorConfigurationCacheTests.cs | 57 +++++++++++++++++++ 3 files changed, 99 insertions(+), 8 deletions(-) diff --git a/MCPForUnity/Editor/McpCiBoot.cs b/MCPForUnity/Editor/McpCiBoot.cs index c8e8c19ea..7fee43edc 100644 --- a/MCPForUnity/Editor/McpCiBoot.cs +++ b/MCPForUnity/Editor/McpCiBoot.cs @@ -1,7 +1,5 @@ -using System; -using MCPForUnity.Editor.Constants; +using MCPForUnity.Editor.Services; using MCPForUnity.Editor.Services.Transport.Transports; -using UnityEditor; namespace MCPForUnity.Editor { @@ -9,9 +7,12 @@ public static class McpCiBoot { public static void StartStdioForCi() { - try - { - EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, false); + // Session-scoped, not EditorPrefs: this must not rewrite the developer's real + // transport preference, and it has to beat the value EditorConfigurationCache + // already read at domain load, which HttpAutoStartHandler consults on its first tick. + try + { + EditorConfigurationCache.Instance.PinStdioForSession(); } catch { /* ignore */ } diff --git a/MCPForUnity/Editor/Services/EditorConfigurationCache.cs b/MCPForUnity/Editor/Services/EditorConfigurationCache.cs index 94f6a397a..a33243fd8 100644 --- a/MCPForUnity/Editor/Services/EditorConfigurationCache.cs +++ b/MCPForUnity/Editor/Services/EditorConfigurationCache.cs @@ -45,7 +45,14 @@ public static EditorConfigurationCache Instance /// public event Action OnConfigurationChanged; + // A headless CI/harness editor (McpCiBoot) must run stdio no matter what the machine-wide + // EditorPrefs say: on Windows those prefs are per-user, so a developer who uses HTTP would + // otherwise have HttpAutoStartHandler stop the stdio bridge the harness is talking to. + // SessionState lives exactly as long as that editor process and survives its domain reloads. + internal const string SessionKeyForceStdio = "MCPForUnity.ForceStdioForSession"; + // Cached values - most frequently read + private bool _forceStdioForSession; private bool _useHttpTransport; private bool _debugLogs; private bool _devModeForceServerRefresh; @@ -59,9 +66,9 @@ public static EditorConfigurationCache Instance /// /// Whether to use HTTP transport (true) or Stdio transport (false). - /// Default: true + /// Default: true. Always false while is in effect. /// - public bool UseHttpTransport => _useHttpTransport; + public bool UseHttpTransport => !_forceStdioForSession && _useHttpTransport; /// /// Whether debug logging is enabled. @@ -128,6 +135,7 @@ private EditorConfigurationCache() /// public void Refresh() { + _forceStdioForSession = SessionState.GetBool(SessionKeyForceStdio, false); _useHttpTransport = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); _debugLogs = EditorPrefs.GetBool(EditorPrefKeys.DebugLogs, false); _devModeForceServerRefresh = EditorPrefs.GetBool(EditorPrefKeys.DevModeForceServerRefresh, false); @@ -140,6 +148,31 @@ public void Refresh() _unitySocketPort = EditorPrefs.GetInt(EditorPrefKeys.UnitySocketPort, 0); } + /// + /// Force stdio transport for the rest of this editor session without touching EditorPrefs. + /// Every UseHttpTransport consumer (auto-start, reload handlers, BridgeControlService, + /// client configurators) sees stdio until or editor exit. + /// + public void PinStdioForSession() + { + SessionState.SetBool(SessionKeyForceStdio, true); + if (!_forceStdioForSession) + { + _forceStdioForSession = true; + OnConfigurationChanged?.Invoke(nameof(UseHttpTransport)); + } + } + + public void UnpinStdioForSession() + { + SessionState.EraseBool(SessionKeyForceStdio); + if (_forceStdioForSession) + { + _forceStdioForSession = false; + OnConfigurationChanged?.Invoke(nameof(UseHttpTransport)); + } + } + /// /// Set UseHttpTransport and update cache + EditorPrefs atomically. /// diff --git a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs index 2991b0c40..d97f6debc 100644 --- a/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs +++ b/TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/EditorConfigurationCacheTests.cs @@ -31,6 +31,7 @@ public void SetUp() public void TearDown() { // Restore original values + EditorConfigurationCache.Instance.UnpinStdioForSession(); EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, _originalUseHttpTransport); EditorPrefs.SetBool(EditorPrefKeys.DebugLogs, _originalDebugLogs); EditorPrefs.SetString(EditorPrefKeys.UvxPathOverride, _originalUvxPath); @@ -260,5 +261,61 @@ public void Refresh_UpdatesAllCachedValues() } #endregion + + #region Session Pin Tests + + [Test] + public void PinStdioForSession_OverridesHttpPreference_WithoutWritingEditorPrefs() + { + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, true); + EditorConfigurationCache.Instance.Refresh(); + Assert.IsTrue(EditorConfigurationCache.Instance.UseHttpTransport); + + EditorConfigurationCache.Instance.PinStdioForSession(); + + Assert.IsFalse(EditorConfigurationCache.Instance.UseHttpTransport); + Assert.IsTrue(EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, false), + "The pin must not rewrite the developer's persisted transport preference"); + } + + [Test] + public void PinStdioForSession_SurvivesRefresh() + { + // Refresh() is what a fresh cache instance runs after a domain reload; the pin lives + // in SessionState precisely so it survives that. + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, true); + EditorConfigurationCache.Instance.PinStdioForSession(); + + EditorConfigurationCache.Instance.Refresh(); + + Assert.IsTrue(SessionState.GetBool(EditorConfigurationCache.SessionKeyForceStdio, false)); + Assert.IsFalse(EditorConfigurationCache.Instance.UseHttpTransport); + } + + [Test] + public void UnpinStdioForSession_RestoresPreferenceAndNotifies() + { + EditorPrefs.SetBool(EditorPrefKeys.UseHttpTransport, true); + EditorConfigurationCache.Instance.Refresh(); + EditorConfigurationCache.Instance.PinStdioForSession(); + + string changedKey = null; + void Handler(string key) => changedKey = key; + EditorConfigurationCache.Instance.OnConfigurationChanged += Handler; + try + { + EditorConfigurationCache.Instance.UnpinStdioForSession(); + } + finally + { + EditorConfigurationCache.Instance.OnConfigurationChanged -= Handler; + } + + Assert.AreEqual(nameof(EditorConfigurationCache.UseHttpTransport), changedKey); + Assert.IsTrue(EditorConfigurationCache.Instance.UseHttpTransport); + Assert.IsFalse(SessionState.GetBool(EditorConfigurationCache.SessionKeyForceStdio, false)); + } + + #endregion } } From e9372a15eaab902b58d8f0486a333f00127a037d Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:41:05 -0400 Subject: [PATCH 4/7] fix(harness): build editor paths with the target platform's separator Discovery is parameterised by platform so it can be tested for every OS from any OS, but it used pathlib.Path, which picks the host separator; four tests failed on Windows with \home\dev\Unity. Use PurePosixPath/PureWindowsPath by target. --- tools/local_harness.py | 35 +++++++++++++++++++++---------- tools/tests/test_local_harness.py | 8 +++---- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/tools/local_harness.py b/tools/local_harness.py index 585a39883..32f39fe74 100644 --- a/tools/local_harness.py +++ b/tools/local_harness.py @@ -69,7 +69,7 @@ import time import xml.etree.ElementTree as ET from dataclasses import dataclass, field -from pathlib import Path +from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath from typing import Any, Callable # --------------------------------------------------------------------------- @@ -230,6 +230,16 @@ def editor_relpath(platform: str | None = None) -> str: return "Editor/Unity" # linux + everything else +def _pure_path(platform: str) -> type[PurePath]: + """Path flavour for the *target* platform, not the host. + + Discovery is parameterised by platform so it can be unit-tested for every OS + from any OS. `Path` would silently use the host's separator, which turns + "/home/dev/Unity" into "\\home\\dev\\Unity" when the tests run on Windows. + """ + return PureWindowsPath if platform.startswith("win") else PurePosixPath + + def hub_roots(platform: str | None = None, environ: dict[str, str] | None = None) -> list[str]: """Per-OS Hub Editor install roots (the directory that holds / dirs). @@ -239,6 +249,7 @@ def hub_roots(platform: str | None = None, environ: dict[str, str] | None = None plat = platform or sys.platform env = environ if environ is not None else os.environ home = env.get("HOME") or env.get("USERPROFILE") or str(Path.home()) + P = _pure_path(plat) if plat == "darwin": return ["/Applications/Unity/Hub/Editor"] if plat.startswith("win"): @@ -246,12 +257,12 @@ def hub_roots(platform: str | None = None, environ: dict[str, str] | None = None for var in ("ProgramFiles", "ProgramFiles(x86)"): base = env.get(var) if base: - roots.append(str(Path(base) / "Unity" / "Hub" / "Editor")) + roots.append(str(P(base) / "Unity" / "Hub" / "Editor")) if not roots: roots.append(r"C:\Program Files\Unity\Hub\Editor") return roots # linux + everything else - return [str(Path(home) / "Unity" / "Hub" / "Editor")] + return [str(P(home) / "Unity" / "Hub" / "Editor")] def read_secondary_install_path(platform: str | None = None, @@ -268,14 +279,15 @@ def read_secondary_install_path(platform: str | None = None, plat = platform or sys.platform env = environ if environ is not None else os.environ home = env.get("HOME") or env.get("USERPROFILE") or str(Path.home()) + P = _pure_path(plat) if plat == "darwin": - cfg = Path(home) / "Library" / "Application Support" / "UnityHub" / "secondaryInstallPath.json" + cfg = P(home) / "Library" / "Application Support" / "UnityHub" / "secondaryInstallPath.json" elif plat.startswith("win"): - appdata = env.get("APPDATA") or str(Path(home) / "AppData" / "Roaming") - cfg = Path(appdata) / "UnityHub" / "secondaryInstallPath.json" + appdata = env.get("APPDATA") or str(P(home) / "AppData" / "Roaming") + cfg = P(appdata) / "UnityHub" / "secondaryInstallPath.json" else: - xdg = env.get("XDG_CONFIG_HOME") or str(Path(home) / ".config") - cfg = Path(xdg) / "UnityHub" / "secondaryInstallPath.json" + xdg = env.get("XDG_CONFIG_HOME") or str(P(home) / ".config") + cfg = P(xdg) / "UnityHub" / "secondaryInstallPath.json" reader = read_text or (lambda p: Path(p).read_text(encoding="utf-8")) try: @@ -310,6 +322,7 @@ def candidate_editor_paths(version: str, plat = platform or sys.platform env = environ if environ is not None else os.environ relpath = editor_relpath(plat) + P = _pure_path(plat) out: list[str] = [] if explicit_editor: @@ -320,11 +333,11 @@ def candidate_editor_paths(version: str, out.append(env_editor) for root in hub_roots(plat, env): - out.append(str(Path(root) / version / relpath)) + out.append(str(P(root) / version / relpath)) sec = read_secondary_install_path(plat, env, read_text) if sec: - out.append(str(Path(sec) / version / relpath)) + out.append(str(P(sec) / version / relpath)) return out @@ -392,7 +405,7 @@ def discover_editor(version: str, pv = parse_version(name) if (pv[0], pv[1]) != (target[0], target[1]): continue - binary = str(Path(root) / name / relpath) + binary = str(_pure_path(plat)(root) / name / relpath) searched.append(binary) if not (_exists(binary) and _is_exec(binary)): continue diff --git a/tools/tests/test_local_harness.py b/tools/tests/test_local_harness.py index 8c3fc1263..1737cfbd3 100644 --- a/tools/tests/test_local_harness.py +++ b/tools/tests/test_local_harness.py @@ -144,7 +144,7 @@ def test_macos_resolves_hub_layout(self): def test_windows_resolves_hub_layout(self): version = "2021.3.45f2" env = {"ProgramFiles": r"C:\Program Files"} - binary = str(Path(r"C:\Program Files") / "Unity" / "Hub" / "Editor" / version / "Editor" / "Unity.exe") + binary = r"C:\Program Files\Unity\Hub\Editor" + f"\\{version}\\Editor\\Unity.exe" exists, is_exec, list_dir = _fake_fs({binary}) spec = resolve_editor_binary( version, @@ -161,7 +161,7 @@ def test_windows_resolves_hub_layout(self): def test_linux_resolves_hub_layout(self): version = "6000.0.75f1" env = {"HOME": "/home/dev"} - binary = str(Path("/home/dev") / "Unity" / "Hub" / "Editor" / version / "Editor" / "Unity") + binary = f"/home/dev/Unity/Hub/Editor/{version}/Editor/Unity" exists, is_exec, list_dir = _fake_fs({binary}) spec = resolve_editor_binary( version, @@ -300,7 +300,7 @@ def test_default_platform_linux(self, monkeypatch): version = "6000.0.75f1" monkeypatch.setattr(lh.sys, "platform", "linux") env = {"HOME": "/home/ci"} - binary = str(Path("/home/ci") / "Unity" / "Hub" / "Editor" / version / "Editor" / "Unity") + binary = f"/home/ci/Unity/Hub/Editor/{version}/Editor/Unity" exists, is_exec, list_dir = _fake_fs({binary}) spec = resolve_editor_binary( version, @@ -319,7 +319,7 @@ class TestSecondaryInstallPath: def test_secondary_root_used_for_candidate(self): version = "6000.0.75f1" secondary = "/Volumes/Big/UnityEditors" - binary = str(Path(secondary) / version / "Unity.app/Contents/MacOS/Unity") + binary = f"{secondary}/{version}/Unity.app/Contents/MacOS/Unity" exists, is_exec, list_dir = _fake_fs({binary}) def read_text(_p: str) -> str: From 9c6b20d824c61382dee67067723851ed5855fedc Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:41:18 -0400 Subject: [PATCH 5/7] fix(server): close three audit findings on the remote-hosted auth surface - /register-tools is no longer registered in remote-hosted mode. It carried no API-key check, so any caller could replace tool definitions for every tenant; the plugin registers tools over the hub WebSocket and never calls this route. - debug_request_context redacts the values of secret-bearing argv flags. It handed --api-key-service-token to every authenticated tenant. - ApiKeyService caps its cache at 1024 entries (expired first, negatives never evict a validated key) so unauthenticated key guesses cannot grow memory, and logs a sha256 fingerprint instead of eight literal characters of the key. --- Server/src/services/api_key_service.py | 29 ++++- Server/src/services/custom_tool_service.py | 7 ++ .../services/tools/debug_request_context.py | 30 ++++- .../tests/integration/test_api_key_service.py | 105 ++++++++++++++++++ .../test_debug_request_context_diagnostics.py | 47 ++++++++ .../test_custom_tool_service_user_scope.py | 23 ++++ 6 files changed, 235 insertions(+), 6 deletions(-) diff --git a/Server/src/services/api_key_service.py b/Server/src/services/api_key_service.py index b0f39909a..2d5afbc6d 100644 --- a/Server/src/services/api_key_service.py +++ b/Server/src/services/api_key_service.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import hashlib import logging import time from dataclasses import dataclass @@ -35,6 +36,11 @@ class ApiKeyService: REQUEST_TIMEOUT: float = 5.0 MAX_RETRIES: int = 1 + # Confirmed-invalid keys are cached too, so a bad key does not re-hit the auth service on + # every call. That also means an unauthenticated caller grows the cache by one entry per + # random key it tries; the cap keeps that bounded and negatives are the first to go. + MAX_CACHE_ENTRIES: int = 1024 + def __init__( self, validation_url: str, @@ -108,24 +114,37 @@ async def validate(self, api_key: str) -> ValidationResult: # not be cached to avoid locking out users during service outages. if result.cacheable: async with self._cache_lock: - expires_at = time.time() + self._cache_ttl + now = time.time() + if len(self._cache) >= self.MAX_CACHE_ENTRIES: + for stale in [k for k, v in self._cache.items() if v[3] <= now]: + del self._cache[stale] + if len(self._cache) >= self.MAX_CACHE_ENTRIES: + if not result.valid: + # Full of live entries: a negative verdict is not worth evicting + # a validated key for. The caller still gets the answer. + return result + del self._cache[min(self._cache, key=lambda k: self._cache[k][3])] self._cache[api_key] = ( result.valid, result.user_id, result.metadata, - expires_at, + now + self._cache_ttl, ) return result + @staticmethod + def _fingerprint(api_key: str) -> str: + """One-way handle for log lines. Eight literal characters of a key were enough to + correlate a leaked log with a key; a hash prefix correlates without exposing any.""" + return "sha256:" + hashlib.sha256(api_key.encode("utf-8")).hexdigest()[:12] + async def _validate_external(self, api_key: str) -> ValidationResult: """Call external validation endpoint. Failure mode: fail closed (treat as invalid on errors). """ - # Redact API key from logs - redacted_key = f"{api_key[:4]}...{api_key[-4:]}" if len( - api_key) > 8 else "***" + redacted_key = self._fingerprint(api_key) for attempt in range(self.MAX_RETRIES + 1): try: diff --git a/Server/src/services/custom_tool_service.py b/Server/src/services/custom_tool_service.py index 04394f33e..f81b7c70e 100644 --- a/Server/src/services/custom_tool_service.py +++ b/Server/src/services/custom_tool_service.py @@ -79,6 +79,13 @@ def get_instance(cls) -> "CustomToolService": # --- HTTP Routes ----------------------------------------------------- def _register_http_routes(self) -> None: + # The plugin registers custom tools over the hub WebSocket (register_tools message), + # so this REST route only serves local tooling. A remote-hosted server must not expose + # it: it carries no API-key check, so any caller could replace tool definitions for + # every tenant. Mirrors the /api/command gate in main.py. + if config.http_remote_hosted: + return + @self._mcp.custom_route("/register-tools", methods=["POST"]) async def register_tools(request: Request) -> JSONResponse: try: diff --git a/Server/src/services/tools/debug_request_context.py b/Server/src/services/tools/debug_request_context.py index 297fb1ead..61eb3c41a 100644 --- a/Server/src/services/tools/debug_request_context.py +++ b/Server/src/services/tools/debug_request_context.py @@ -11,6 +11,34 @@ from transport.unity_instance_middleware import get_unity_instance_middleware from transport.plugin_hub import PluginHub +_SECRET_FLAG_MARKERS = ("token", "secret", "password", "api-key", "api_key", "apikey") + + +def _redact_argv(argv: list[str]) -> list[str]: + """Keep flag names for diagnosis, hide the values of secret-bearing ones. + + A remote-hosted server is started with --api-key-service-token on its command line and + this tool is callable by every authenticated tenant, so the raw argv handed out the + service credential. The flag shape is what helps debug a deployment; the value never is. + """ + out: list[str] = [] + hide_next = False + for arg in argv: + if hide_next: + hide_next = False + if not arg.startswith("-"): + out.append("***") + continue + name, sep, _value = arg.partition("=") + lowered = name.lower() + secret = name.startswith("-") and any(m in lowered for m in _SECRET_FLAG_MARKERS) + if secret and sep: + out.append(f"{name}=***") + else: + out.append(arg) + hide_next = secret + return out + @mcp_for_unity_tool( unity_target=None, @@ -65,7 +93,7 @@ async def debug_request_context(ctx: Context) -> dict[str, Any]: "server": { "version": get_package_version(), "cwd": os.getcwd(), - "argv": list(sys.argv), + "argv": _redact_argv(sys.argv), }, "request_context": { "client_id": rc_client_id, diff --git a/Server/tests/integration/test_api_key_service.py b/Server/tests/integration/test_api_key_service.py index 93af8bd73..1cc4d8c5f 100644 --- a/Server/tests/integration/test_api_key_service.py +++ b/Server/tests/integration/test_api_key_service.py @@ -454,3 +454,108 @@ async def capture_post(url, *, json=None, headers=None): assert captured_headers.get("X-Service-Token") == "test-svc-token-123" assert captured_headers.get("Content-Type") == "application/json" + + +# --------------------------------------------------------------------------- +# Cache bound + log redaction +# --------------------------------------------------------------------------- + +def _patched_client(mock_resp): + ctx = patch("httpx.AsyncClient") + MockClient = ctx.start() + instance = AsyncMock() + instance.__aenter__ = AsyncMock(return_value=instance) + instance.__aexit__ = AsyncMock(return_value=False) + instance.post = AsyncMock(return_value=mock_resp) + MockClient.return_value = instance + return ctx, instance + + +class TestCacheBound: + @pytest.mark.asyncio + async def test_negative_results_cannot_grow_cache_past_cap(self, monkeypatch): + """Unauthenticated callers choose the key, so every failed guess used to add an + entry. The cap must hold no matter how many distinct bad keys arrive.""" + monkeypatch.setattr(ApiKeyService, "MAX_CACHE_ENTRIES", 5) + svc = _make_service() + ctx, _ = _patched_client(_mock_response(401)) + try: + for i in range(50): + result = await svc.validate(f"bad-key-{i:04d}-padding-to-length") + assert result.valid is False + finally: + ctx.stop() + assert len(svc._cache) <= 5 + + @pytest.mark.asyncio + async def test_valid_key_still_cached_when_cap_is_full_of_negatives(self, monkeypatch): + monkeypatch.setattr(ApiKeyService, "MAX_CACHE_ENTRIES", 3) + svc = _make_service() + ctx, instance = _patched_client(_mock_response(401)) + try: + for i in range(3): + await svc.validate(f"bad-key-{i:04d}-padding-to-length") + instance.post = AsyncMock(return_value=_mock_response( + 200, {"valid": True, "user_id": "user-1"})) + r1 = await svc.validate("good-key-000-padding-to-length") + calls_after_first = instance.post.await_count + r2 = await svc.validate("good-key-000-padding-to-length") + finally: + ctx.stop() + assert r1.valid and r2.valid + # Second call was served from cache: a validated key evicts a negative entry. + assert instance.post.await_count == calls_after_first + assert len(svc._cache) <= 3 + assert "good-key-000-padding-to-length" in svc._cache + + @pytest.mark.asyncio + async def test_expired_entries_are_purged_before_evicting_live_ones(self, monkeypatch): + monkeypatch.setattr(ApiKeyService, "MAX_CACHE_ENTRIES", 2) + svc = _make_service() + ctx, _ = _patched_client(_mock_response(200, {"valid": True, "user_id": "u"})) + try: + await svc.validate("live-key-aaaa-padding-to-length") + await svc.validate("stale-key-bbbb-padding-to-length") + async with svc._cache_lock: + v = svc._cache["stale-key-bbbb-padding-to-length"] + svc._cache["stale-key-bbbb-padding-to-length"] = (v[0], v[1], v[2], time.time() - 1) + await svc.validate("new-key-cccc-padding-to-length") + finally: + ctx.stop() + assert "live-key-aaaa-padding-to-length" in svc._cache + assert "stale-key-bbbb-padding-to-length" not in svc._cache + assert "new-key-cccc-padding-to-length" in svc._cache + + +class TestLogRedaction: + KEY = "sk-live-ABCDEFGHIJKLMNOPQRSTUVWXYZ" + + def test_fingerprint_contains_no_key_characters_and_is_stable(self): + fp = ApiKeyService._fingerprint(self.KEY) + assert fp.startswith("sha256:") + assert self.KEY[:4] not in fp and self.KEY[-4:] not in fp + assert fp == ApiKeyService._fingerprint(self.KEY) + assert fp != ApiKeyService._fingerprint(self.KEY + "x") + + @pytest.mark.asyncio + async def test_warning_on_auth_service_error_does_not_log_key_fragments(self): + # Assert on the logger call itself rather than captured text: other test modules + # reconfigure the "mcp-for-unity-server" logger, which makes caplog order-dependent. + svc = _make_service() + ctx, _ = _patched_client(_mock_response(500)) + with patch("services.api_key_service.logger") as mock_logger: + try: + result = await svc.validate(self.KEY) + finally: + ctx.stop() + assert result.valid is False + assert mock_logger.warning.called + rendered = [ + (call.args[0] % tuple(call.args[1:])) if len(call.args) > 1 else str(call.args[0]) + for call in mock_logger.warning.call_args_list + ] + assert any("API key validation returned status 500" in line for line in rendered) + for line in rendered: + assert self.KEY not in line + assert self.KEY[:4] not in line + assert self.KEY[-4:] not in line diff --git a/Server/tests/integration/test_debug_request_context_diagnostics.py b/Server/tests/integration/test_debug_request_context_diagnostics.py index 3ebccae07..0b541c1d5 100644 --- a/Server/tests/integration/test_debug_request_context_diagnostics.py +++ b/Server/tests/integration/test_debug_request_context_diagnostics.py @@ -27,4 +27,51 @@ async def get_state(self, _k): assert "argv" in server +@pytest.mark.asyncio +async def test_debug_request_context_redacts_secret_argv(monkeypatch): + """Any tenant can call this tool on a remote-hosted server, so the service token that + the server was started with must not come back in the diagnostics.""" + import json + import services.tools.debug_request_context as mod + + class DummyCtx: + request_context = None + session_id = None + client_id = None + + async def get_state(self, _k): + return None + + monkeypatch.setattr(mod, "get_package_version", lambda: "9.9.9-test") + monkeypatch.setattr(mod.sys, "argv", [ + "mcp-for-unity", + "--http-remote-hosted", + "--api-key-service-token", "s3cret-service-token", + "--api-key-validation-url=https://auth.example.com/validate?token=url-embedded", + "--api-key-service-token-header", "X-Service-Token", + "--http-port", "8080", + ]) + + res = await mod.debug_request_context(DummyCtx()) + argv = res["data"]["server"]["argv"] + dumped = json.dumps(argv) + + assert "s3cret-service-token" not in dumped + assert "url-embedded" not in dumped + # Flag names and non-secret values survive so the deployment shape stays debuggable. + assert "--api-key-service-token" in argv + assert "--http-remote-hosted" in argv + assert argv[-2:] == ["--http-port", "8080"] + + +def test_redact_argv_does_not_swallow_a_following_flag(): + import services.tools.debug_request_context as mod + + # A secret-looking flag used as a bare switch must not hide the flag after it. + assert mod._redact_argv(["--token", "--verbose"]) == ["--token", "--verbose"] + assert mod._redact_argv(["--token", "abc", "--verbose"]) == ["--token", "***", "--verbose"] + assert mod._redact_argv(["--password=hunter2"]) == ["--password=***"] + assert mod._redact_argv(["positional", "--port", "1"]) == ["positional", "--port", "1"] + + diff --git a/Server/tests/test_custom_tool_service_user_scope.py b/Server/tests/test_custom_tool_service_user_scope.py index ac3915a7f..d1f2c648a 100644 --- a/Server/tests/test_custom_tool_service_user_scope.py +++ b/Server/tests/test_custom_tool_service_user_scope.py @@ -17,6 +17,29 @@ def _decorator(fn): return _decorator +class _RecordingMcp(_DummyMcp): + def __init__(self): + self.routes: list[str] = [] + + def custom_route(self, path, methods=None): # noqa: ARG002 + self.routes.append(path) + return super().custom_route(path, methods) + + +def test_register_tools_route_is_not_exposed_in_remote_hosted_mode(monkeypatch): + """The REST route has no API-key check; the plugin registers tools over the hub + WebSocket instead, so a hosted server must not offer it to unauthenticated callers.""" + monkeypatch.setattr(config, "http_remote_hosted", True) + hosted = _RecordingMcp() + CustomToolService(hosted) + assert "/register-tools" not in hosted.routes + + monkeypatch.setattr(config, "http_remote_hosted", False) + local = _RecordingMcp() + CustomToolService(local) + assert local.routes == ["/register-tools"] + + @pytest.mark.asyncio async def test_list_registered_tools_threads_user_id_to_plugin_hub(): service = CustomToolService(_DummyMcp()) From e87e5d0a34f475847778a92f8108cbbfe96593f2 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:41:18 -0400 Subject: [PATCH 6/7] ci: stop xtrace from echoing Unity credentials during activation set -x printed the expanded -password/-serial arguments into the job log and left GitHub's secret masking as the only protection. --- .github/workflows/e2e-bridge.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/e2e-bridge.yml b/.github/workflows/e2e-bridge.yml index 4cb9d7018..62f8f305f 100644 --- a/.github/workflows/e2e-bridge.yml +++ b/.github/workflows/e2e-bridge.yml @@ -146,7 +146,9 @@ jobs: -v "$RUNNER_TEMP/unity-config:/root/.config/unity3d" \ -v "$RUNNER_TEMP/unity-local:/root/.local/share/unity3d" \ "$UNITY_IMAGE" bash -lc ' - set -euxo pipefail + # No -x here: xtrace would echo the expanded -password/-serial arguments into + # the job log, and GitHub secret masking is a last line of defence, not a design. + set -euo pipefail /opt/unity/Editor/Unity -batchmode -nographics -logFile - \ -username "$UNITY_EMAIL" -password "$UNITY_PASSWORD" -serial "$UNITY_SERIAL" -quit || true ' From a4a003e40be2283a145a6ead54a29e6e22fbe091 Mon Sep 17 00:00:00 2001 From: Shutong Wu <51266340+Scriptwonder@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:46:31 -0400 Subject: [PATCH 7/7] fix: address review comments on the hardening PR - _redact_argv now always hides the token after a bare secret flag; every secret flag the server accepts takes a value, so a value starting with '-' is still the secret (Copilot). - ApiKeyService evicts a negative entry before any validated key when a new validated key needs room, matching the comment's intent (Copilot). - compile-check.sh normalises EXTRA_REFS through winpath so an MSYS-style /c/refs reaches Roslyn as C:/refs (CodeRabbit). - CLAUDE.md documents the EXTRA_REFS prerequisite (CodeRabbit). --- CLAUDE.md | 6 ++++-- Server/src/services/api_key_service.py | 8 ++++++-- .../services/tools/debug_request_context.py | 17 +++++++---------- .../tests/integration/test_api_key_service.py | 19 +++++++++++++++++++ .../test_debug_request_context_diagnostics.py | 6 +++--- tools/compile-check.sh | 1 + 6 files changed, 40 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 549a927a7..d95638a4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -167,8 +167,10 @@ tools/check-unity-versions.sh # compile-only across installed Unity Hu tools/check-unity-versions.sh --full # full EditMode test run # License-free Roslyn compile of MCPForUnity, the same gate compile-check.yml runs on every PR. -# No Editor launch, ~1 min per version; the script header has the Windows/Git Bash recipe. -UNITY_DATA=/path/to/Editor/Data UNITY_VERSION=2021.3.45f2 tools/compile-check.sh +# No Editor launch, ~1 min per version. EXTRA_REFS must hold Newtonsoft.Json.dll and +# nunit.framework.dll (copy them from TestProjects/UnityMCPTests/Library/PackageCache); +# the script header has the full recipe, including the Windows/Git Bash form. +UNITY_DATA=/path/to/Editor/Data UNITY_VERSION=2021.3.45f2 EXTRA_REFS=/path/to/refs tools/compile-check.sh ``` #### Local headless test harness diff --git a/Server/src/services/api_key_service.py b/Server/src/services/api_key_service.py index 2d5afbc6d..9d6881f7e 100644 --- a/Server/src/services/api_key_service.py +++ b/Server/src/services/api_key_service.py @@ -121,9 +121,13 @@ async def validate(self, api_key: str) -> ValidationResult: if len(self._cache) >= self.MAX_CACHE_ENTRIES: if not result.valid: # Full of live entries: a negative verdict is not worth evicting - # a validated key for. The caller still gets the answer. + # anything for. The caller still gets the answer. return result - del self._cache[min(self._cache, key=lambda k: self._cache[k][3])] + # Make room for a validated key: drop a negative entry if there is + # one, otherwise the validated key that expires soonest. + negatives = [k for k, v in self._cache.items() if not v[0]] + pool = negatives or list(self._cache) + del self._cache[min(pool, key=lambda k: self._cache[k][3])] self._cache[api_key] = ( result.valid, result.user_id, diff --git a/Server/src/services/tools/debug_request_context.py b/Server/src/services/tools/debug_request_context.py index 61eb3c41a..4baa77a5f 100644 --- a/Server/src/services/tools/debug_request_context.py +++ b/Server/src/services/tools/debug_request_context.py @@ -21,22 +21,19 @@ def _redact_argv(argv: list[str]) -> list[str]: this tool is callable by every authenticated tenant, so the raw argv handed out the service credential. The flag shape is what helps debug a deployment; the value never is. """ + # Every secret-bearing flag the server accepts takes a value, so the token after a bare + # flag is always that value, even when it happens to start with "-". out: list[str] = [] hide_next = False for arg in argv: if hide_next: + out.append("***") hide_next = False - if not arg.startswith("-"): - out.append("***") - continue + continue name, sep, _value = arg.partition("=") - lowered = name.lower() - secret = name.startswith("-") and any(m in lowered for m in _SECRET_FLAG_MARKERS) - if secret and sep: - out.append(f"{name}=***") - else: - out.append(arg) - hide_next = secret + secret = name.startswith("-") and any(m in name.lower() for m in _SECRET_FLAG_MARKERS) + out.append(f"{name}=***" if secret and sep else arg) + hide_next = secret and not sep return out diff --git a/Server/tests/integration/test_api_key_service.py b/Server/tests/integration/test_api_key_service.py index 1cc4d8c5f..92dc1755d 100644 --- a/Server/tests/integration/test_api_key_service.py +++ b/Server/tests/integration/test_api_key_service.py @@ -559,3 +559,22 @@ async def test_warning_on_auth_service_error_does_not_log_key_fragments(self): assert self.KEY not in line assert self.KEY[:4] not in line assert self.KEY[-4:] not in line + + @pytest.mark.asyncio + async def test_new_valid_key_evicts_a_negative_before_any_valid_entry(self, monkeypatch): + monkeypatch.setattr(ApiKeyService, "MAX_CACHE_ENTRIES", 3) + svc = _make_service() + ctx, instance = _patched_client(_mock_response(200, {"valid": True, "user_id": "u"})) + try: + await svc.validate("valid-key-aaaa-padding-to-length") + await svc.validate("valid-key-bbbb-padding-to-length") + instance.post = AsyncMock(return_value=_mock_response(401)) + await svc.validate("bad-key-cccc-padding-to-length") + instance.post = AsyncMock(return_value=_mock_response(200, {"valid": True, "user_id": "u"})) + await svc.validate("valid-key-dddd-padding-to-length") + finally: + ctx.stop() + assert "bad-key-cccc-padding-to-length" not in svc._cache + assert "valid-key-aaaa-padding-to-length" in svc._cache + assert "valid-key-bbbb-padding-to-length" in svc._cache + assert "valid-key-dddd-padding-to-length" in svc._cache diff --git a/Server/tests/integration/test_debug_request_context_diagnostics.py b/Server/tests/integration/test_debug_request_context_diagnostics.py index 0b541c1d5..980b161ba 100644 --- a/Server/tests/integration/test_debug_request_context_diagnostics.py +++ b/Server/tests/integration/test_debug_request_context_diagnostics.py @@ -64,11 +64,11 @@ async def get_state(self, _k): assert argv[-2:] == ["--http-port", "8080"] -def test_redact_argv_does_not_swallow_a_following_flag(): +def test_redact_argv_hides_the_value_even_when_it_starts_with_a_dash(): import services.tools.debug_request_context as mod - # A secret-looking flag used as a bare switch must not hide the flag after it. - assert mod._redact_argv(["--token", "--verbose"]) == ["--token", "--verbose"] + # Secret flags always take a value; a value that begins with "-" is still the secret. + assert mod._redact_argv(["--token", "-abc123", "--verbose"]) == ["--token", "***", "--verbose"] assert mod._redact_argv(["--token", "abc", "--verbose"]) == ["--token", "***", "--verbose"] assert mod._redact_argv(["--password=hunter2"]) == ["--password=***"] assert mod._redact_argv(["positional", "--port", "1"]) == ["positional", "--port", "1"] diff --git a/tools/compile-check.sh b/tools/compile-check.sh index 8d7331077..7d7bdf696 100644 --- a/tools/compile-check.sh +++ b/tools/compile-check.sh @@ -46,6 +46,7 @@ winpath() { (cd "$1" 2>/dev/null && (pwd -W 2>/dev/null || pwd)) || die "directo UNITY_DATA=$(winpath "${UNITY_DATA:-/opt/unity/Editor/Data}") REPO=$(winpath "${REPO:-"$(dirname "${BASH_SOURCE[0]}")/.."}") EXTRA_REFS=${EXTRA_REFS:-"$REPO/.compile-refs"} +[ -d "$EXTRA_REFS" ] && EXTRA_REFS=$(winpath "$EXTRA_REFS") PLATFORMS=${PLATFORMS:-"win osx linux"} OUT=${OUT:-/tmp/mcp-compile-check} mkdir -p "$OUT" && OUT=$(winpath "$OUT")