Skip to content

Commit 2efb786

Browse files
committed
fix(asset-gen): security hardening + provider correctness + local image_path
Security review + code review of the asset-gen feature surfaced concrete issues; this fixes them and adds regression tests (request-shaping layer, FakeHttpTransport). Security - SafeZipExtractor enforces an extension allowlist; ModelImportPipeline passes an inert model/texture allowlist so a provider archive can't drop a .cs/.dll under Assets/ and have the Editor compile/load it (code execution on import). - AssetGenJobManager refuses non-http(s) download URLs before fetching (file:// SSRF / local-file read into the project). Provider correctness - Meshy image->3D polls /openapi/v1/image-to-3d/{id} (was the v2 text URL). - Meshy text->3D honors texture=true via the preview->refine two-phase flow. - OpenRouter image->image attaches the reference image (content image_url part). - fal image->image uses the /edit endpoint + image_urls array; width/height forwarded as image_size. - Sketchfab search forwards categories/count/cursor/downloadable; preview doc corrected (returns metadata, not a base64 thumbnail). - Job import calls AssetDatabase.Refresh() before importing a freshly written file. Local image input (image_path) - New LocalImage helper; image_path is read and sent inline as a base64 data URI for Meshy / fal / OpenRouter. Tripo rejects local images with a clear error (needs a hosted image_url; its upload flow is not wired). Cleanup (no behavior change) - Shared AssetGenPaths + ProviderHttp helpers, HttpResult.Ok, MissingKeyMessage, cached glTFast probe, dead-field / per-frame-alloc removal, CLI _emit. Docs: README + manual-verification updated (image_path support; transparency is import-flag-only; width/height fal-only). Verified: package compiles clean; Python 1306 passed / 3 skipped. Meshy refine, fal /edit, and image_path data-URI paths are unit-tested at the request layer only -- live smoke per provider (real keys) still pending. Claude-Session: https://claude.ai/code/session_015DAUrMR5UaSEzEn2wNPrEP
1 parent e243e30 commit 2efb786

37 files changed

Lines changed: 771 additions & 244 deletions
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
using System.IO;
2+
using UnityEngine;
3+
4+
namespace MCPForUnity.Editor.Helpers
5+
{
6+
/// <summary>
7+
/// Project-path conversions shared by the asset-gen import/write code: project-relative
8+
/// ("Assets/...") ↔ absolute on-disk paths, with forward-slash normalization for
9+
/// cross-platform consistency.
10+
/// </summary>
11+
public static class AssetGenPaths
12+
{
13+
/// <summary>Resolve a project-relative ("Assets/...") path to an absolute, forward-slashed path.</summary>
14+
public static string ToAbsolute(string projectRelative)
15+
{
16+
string dataPath = Application.dataPath.Replace('\\', '/');
17+
string projectRoot = dataPath.Substring(0, dataPath.Length - "Assets".Length);
18+
return Path.Combine(projectRoot, projectRelative).Replace('\\', '/');
19+
}
20+
21+
/// <summary>Convert an absolute (or already-relative) path to a project-relative ("Assets/...") path.</summary>
22+
public static string ToProjectRelative(string path)
23+
{
24+
string p = path.Replace('\\', '/');
25+
if (p.StartsWith("Assets")) return p;
26+
string dataPath = Application.dataPath.Replace('\\', '/');
27+
if (p.StartsWith(dataPath)) return "Assets" + p.Substring(dataPath.Length);
28+
return p;
29+
}
30+
}
31+
}

MCPForUnity/Editor/Helpers/AssetGenPaths.cs.meta

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

MCPForUnity/Editor/Services/AssetGen/AssetGenJobManager.cs

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ public static class AssetGenJobManager
5858

5959
private static readonly Dictionary<string, AssetGenJob> Jobs = new();
6060
private static readonly Dictionary<string, Runner> Runners = new();
61+
private static readonly List<string> _tickIds = new();
6162
private static bool _ticking;
6263

6364
static AssetGenJobManager()
@@ -221,7 +222,6 @@ private sealed class Runner
221222
public double NextPollAt;
222223
public string ProviderJobId;
223224
public string DownloadUrl;
224-
public byte[] InlineData;
225225
public string LocalPath;
226226
public Task<string> SubmitTask;
227227
public Task<ProviderPollResult> PollTask;
@@ -234,7 +234,7 @@ private static bool TryResolveKey(string provider, AssetGenJob job, out string a
234234
if (!SecureKeyStore.Current.TryGet(provider, out apiKey) || string.IsNullOrEmpty(apiKey))
235235
{
236236
job.State = AssetGenJobState.Failed;
237-
job.Error = $"No API key configured for '{provider}'. Add it in the MCP for Unity → Asset Generation tab.";
237+
job.Error = AssetGenProviders.MissingKeyMessage(provider);
238238
Jobs[job.JobId] = job;
239239
Persist(job);
240240
return false;
@@ -266,7 +266,11 @@ private static void Tick()
266266
_ticking = false;
267267
return;
268268
}
269-
foreach (string id in new List<string>(Runners.Keys))
269+
// Snapshot keys into a reused buffer so Advance can mutate Runners mid-iteration
270+
// without churning the GC on every editor-update frame.
271+
_tickIds.Clear();
272+
_tickIds.AddRange(Runners.Keys);
273+
foreach (string id in _tickIds)
270274
{
271275
if (Runners.TryGetValue(id, out var r)) Advance(r);
272276
}
@@ -353,6 +357,14 @@ private static void Advance(Runner r)
353357
break;
354358

355359
case RunnerPhase.Download:
360+
// The download URL comes from an untrusted provider response. Only fetch
361+
// http(s) — refuse file://, ftp://, etc. so a malicious response can't read
362+
// a local file into the project or hit an internal host.
363+
if (!IsAllowedDownloadUrl(r.DownloadUrl))
364+
{
365+
Fail(r, "Refusing to fetch a non-http(s) download URL returned by the provider.");
366+
break;
367+
}
356368
r.DownloadTask = r.Transport.SendAsync(
357369
new HttpRequestSpec { Method = "GET", Url = r.DownloadUrl }, r.Cts.Token);
358370
r.Phase = RunnerPhase.AwaitDownload;
@@ -374,6 +386,10 @@ private static void Advance(Runner r)
374386
break;
375387

376388
case RunnerPhase.Import:
389+
// The result file was just written via File.WriteAllBytes (outside the
390+
// AssetDatabase). Refresh so Unity registers it before we import it,
391+
// mirroring ImportModelFile. Skipped under the test import seam.
392+
if (ImportOverrideForTests == null) AssetDatabase.Refresh();
377393
AssetGenJob imported = r.ImportFn(r.Job, r.LocalPath);
378394
if (imported != null) r.Job = imported;
379395
if (r.Job.State != AssetGenJobState.Failed)
@@ -399,7 +415,7 @@ private static string WriteFile(Runner r, byte[] bytes)
399415
string root = !string.IsNullOrEmpty(r.OutputFolder) ? r.OutputFolder
400416
: (AssetGenPrefs.OutputRoot + "/" + r.Subfolder);
401417
if (!root.Replace('\\', '/').StartsWith("Assets")) root = AssetGenPrefs.OutputRoot + "/" + r.Subfolder;
402-
string absRoot = ToAbsolute(root);
418+
string absRoot = AssetGenPaths.ToAbsolute(root);
403419
Directory.CreateDirectory(absRoot);
404420
string baseName = SanitizeName(r.Name);
405421
string fileName = baseName + "." + ext;
@@ -417,13 +433,6 @@ private static string NameFrom(string explicitName, string prompt, string jobId)
417433
return "asset_" + jobId.Substring(0, 8);
418434
}
419435

420-
private static string ToAbsolute(string projectRelative)
421-
{
422-
string dataPath = Application.dataPath;
423-
string projectRoot = dataPath.Substring(0, dataPath.Length - "Assets".Length);
424-
return Path.Combine(projectRoot, projectRelative);
425-
}
426-
427436
private static string SanitizeName(string raw)
428437
{
429438
if (string.IsNullOrWhiteSpace(raw)) return "asset";
@@ -494,6 +503,11 @@ private static bool Faulted(Task t, out string error)
494503
private static bool IsTerminal(AssetGenJobState s)
495504
=> s == AssetGenJobState.Done || s == AssetGenJobState.Failed || s == AssetGenJobState.Canceled;
496505

506+
/// <summary>Only http(s) download URLs are allowed; provider responses are untrusted.</summary>
507+
private static bool IsAllowedDownloadUrl(string url)
508+
=> Uri.TryCreate(url, UriKind.Absolute, out Uri u)
509+
&& (u.Scheme == Uri.UriSchemeHttps || u.Scheme == Uri.UriSchemeHttp);
510+
497511
private static double Now() => EditorApplication.timeSinceStartup;
498512

499513
internal static void ResetForTests()

MCPForUnity/Editor/Services/AssetGen/Http/HttpResult.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,5 +12,8 @@ public sealed class HttpResult
1212
public byte[] Body;
1313
public string Text;
1414
public bool IsSuccess;
15+
16+
/// <summary>True when the transport reports success or the status code is 2xx.</summary>
17+
public bool Ok => IsSuccess || (Status >= 200 && Status < 300);
1518
}
1619
}

MCPForUnity/Editor/Services/AssetGen/Import/ImageImportPipeline.cs

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
using System;
22
using System.IO;
3+
using MCPForUnity.Editor.Helpers;
34
using MCPForUnity.Editor.Security;
45
using UnityEditor;
5-
using UnityEngine;
66

77
namespace MCPForUnity.Editor.Services.AssetGen.Import
88
{
@@ -20,7 +20,7 @@ public static AssetGenJob ImportInto(AssetGenJob job, string localFilePath, bool
2020
if (string.IsNullOrEmpty(localFilePath))
2121
return Fail(job, "No file to import.");
2222

23-
string rel = ToProjectRelative(localFilePath);
23+
string rel = AssetGenPaths.ToProjectRelative(localFilePath);
2424
if (string.IsNullOrEmpty(rel) || !rel.Replace('\\', '/').StartsWith("Assets"))
2525
return Fail(job, "Generated file is not under the Assets folder.");
2626

@@ -54,15 +54,6 @@ public static AssetGenJob ImportInto(AssetGenJob job, string localFilePath, bool
5454
}
5555
}
5656

57-
private static string ToProjectRelative(string path)
58-
{
59-
string p = path.Replace('\\', '/');
60-
if (p.StartsWith("Assets")) return p;
61-
string dataPath = Application.dataPath.Replace('\\', '/');
62-
if (p.StartsWith(dataPath)) return "Assets" + p.Substring(dataPath.Length);
63-
return p;
64-
}
65-
6657
private static AssetGenJob Fail(AssetGenJob job, string message)
6758
{
6859
job.State = AssetGenJobState.Failed;

MCPForUnity/Editor/Services/AssetGen/Import/ModelImportPipeline.cs

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Generic;
23
using System.IO;
34
using MCPForUnity.Editor.Helpers;
45
using MCPForUnity.Editor.Security;
@@ -15,6 +16,15 @@ namespace MCPForUnity.Editor.Services.AssetGen.Import
1516
/// </summary>
1617
public static class ModelImportPipeline
1718
{
19+
// Inert asset types permitted out of an UNTRUSTED provider archive (Sketchfab et al.).
20+
// Anything else — scripts, assemblies, asmdefs — is skipped on extraction so it can never
21+
// compile or load inside the Editor. See SafeZipExtractor for the enforcement.
22+
private static readonly HashSet<string> ArchiveAllowedExtensions = new(StringComparer.OrdinalIgnoreCase)
23+
{
24+
".gltf", ".glb", ".bin", ".fbx", ".obj", ".mtl",
25+
".png", ".jpg", ".jpeg", ".tga", ".bmp", ".tif", ".tiff", ".webp", ".exr", ".hdr", ".ktx2", ".basis",
26+
};
27+
1828
public static AssetGenJob ImportInto(AssetGenJob job, string localFilePath)
1929
{
2030
if (job == null) return null;
@@ -23,7 +33,7 @@ public static AssetGenJob ImportInto(AssetGenJob job, string localFilePath)
2333
if (string.IsNullOrEmpty(localFilePath))
2434
return Fail(job, "No file to import.");
2535

26-
string rel = ToProjectRelative(localFilePath);
36+
string rel = AssetGenPaths.ToProjectRelative(localFilePath);
2737
if (string.IsNullOrEmpty(rel) || !rel.Replace('\\', '/').StartsWith("Assets"))
2838
return Fail(job, "Generated file is not under the Assets folder.");
2939

@@ -71,15 +81,17 @@ private static AssetGenJob ImportModelFile(AssetGenJob job, string rel, string e
7181
/// </summary>
7282
private static AssetGenJob ImportArchive(AssetGenJob job, string zipRel)
7383
{
74-
string zipAbs = ToAbsolute(zipRel);
84+
string zipAbs = AssetGenPaths.ToAbsolute(zipRel);
7585
if (!File.Exists(zipAbs))
7686
return Fail(job, "Downloaded archive was not found on disk.");
7787

7888
string folderRel = zipRel.Substring(0, zipRel.Length - ".zip".Length);
79-
string folderAbs = ToAbsolute(folderRel);
89+
string folderAbs = AssetGenPaths.ToAbsolute(folderRel);
8090

8191
Directory.CreateDirectory(folderAbs);
82-
SafeZipExtractor.ExtractTo(zipAbs, folderAbs);
92+
// Provider archives are untrusted: only inert model/texture files are written under
93+
// Assets/ — scripts/assemblies are skipped so they can't be compiled on import.
94+
SafeZipExtractor.ExtractTo(zipAbs, folderAbs, ArchiveAllowedExtensions);
8395

8496
AssetDatabase.Refresh();
8597
AssetDatabase.ImportAsset(folderRel, ImportAssetOptions.ImportRecursive | ImportAssetOptions.ForceUpdate);
@@ -126,18 +138,11 @@ private static string FindFirstModel(string folderAbs)
126138
{
127139
string e = Path.GetExtension(abs).ToLowerInvariant();
128140
if (e == ".fbx" || e == ".obj")
129-
return ToProjectRelative(abs);
141+
return AssetGenPaths.ToProjectRelative(abs);
130142
if (firstGltf == null && (e == ".glb" || e == ".gltf"))
131143
firstGltf = abs;
132144
}
133-
return firstGltf == null ? null : ToProjectRelative(firstGltf);
134-
}
135-
136-
private static string ToAbsolute(string projectRelative)
137-
{
138-
string dataPath = Application.dataPath.Replace('\\', '/');
139-
string projectRoot = dataPath.Substring(0, dataPath.Length - "Assets".Length);
140-
return Path.Combine(projectRoot, projectRelative).Replace('\\', '/');
145+
return firstGltf == null ? null : AssetGenPaths.ToProjectRelative(firstGltf);
141146
}
142147

143148
private static void ApplyModelImporterSettings(string rel, AssetGenJob job)
@@ -191,24 +196,27 @@ private static float ComputeMaxDimension(string rel)
191196
catch { return 0f; }
192197
}
193198

194-
private static bool IsGltfastAvailable()
199+
private static bool? _gltfastAvailable;
200+
201+
/// <summary>
202+
/// True when the glTFast package is present. Cached after the first probe — the result only
203+
/// changes on a package install/uninstall, which triggers a domain reload that resets this
204+
/// static. Shared with the Asset Gen settings tab so the reflection scan runs at most once.
205+
/// </summary>
206+
internal static bool IsGltfastAvailable()
195207
{
196-
if (Type.GetType("GLTFast.GltfImport, glTFast") != null) return true;
197-
foreach (var asm in AppDomain.CurrentDomain.GetAssemblies())
208+
if (_gltfastAvailable.HasValue) return _gltfastAvailable.Value;
209+
bool found = Type.GetType("GLTFast.GltfImport, glTFast") != null;
210+
if (!found)
198211
{
199-
try { if (asm.GetType("GLTFast.GltfImport") != null) return true; }
200-
catch { /* dynamic/!resolvable assembly */ }
212+
foreach (var asm in AppDomain.CurrentDomain.GetAssemblies())
213+
{
214+
try { if (asm.GetType("GLTFast.GltfImport") != null) { found = true; break; } }
215+
catch { /* dynamic/!resolvable assembly */ }
216+
}
201217
}
202-
return false;
203-
}
204-
205-
private static string ToProjectRelative(string path)
206-
{
207-
string p = path.Replace('\\', '/');
208-
if (p.StartsWith("Assets")) return p;
209-
string dataPath = Application.dataPath.Replace('\\', '/');
210-
if (p.StartsWith(dataPath)) return "Assets" + p.Substring(dataPath.Length);
211-
return p;
218+
_gltfastAvailable = found;
219+
return found;
212220
}
213221

214222
private static AssetGenJob Fail(AssetGenJob job, string message)

MCPForUnity/Editor/Services/AssetGen/Import/SafeZipExtractor.cs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Collections.Generic;
23
using System.IO;
34
using System.IO.Compression;
45

@@ -9,10 +10,15 @@ namespace MCPForUnity.Editor.Services.AssetGen.Import
910
/// every entry's resolved target must stay inside <c>destDir</c>. Directory entries are
1011
/// created; file entries are written by copying the entry stream (no reliance on the
1112
/// ZipFileExtensions helper). Used to unpack marketplace model archives (e.g. Sketchfab).
13+
///
14+
/// When <paramref name="allowedExtensions"/> is supplied, file entries whose extension is not
15+
/// on the allowlist are SKIPPED (not written). Callers that extract UNTRUSTED archives into the
16+
/// Assets tree MUST pass an allowlist of inert asset types so executable content (.cs/.dll/
17+
/// .asmdef) can never land under Assets/ and be compiled/loaded by the Editor.
1218
/// </summary>
1319
public static class SafeZipExtractor
1420
{
15-
public static void ExtractTo(string zipPath, string destDir)
21+
public static void ExtractTo(string zipPath, string destDir, ISet<string> allowedExtensions = null)
1622
{
1723
if (string.IsNullOrEmpty(zipPath)) throw new ArgumentException("zipPath required", nameof(zipPath));
1824
if (string.IsNullOrEmpty(destDir)) throw new ArgumentException("destDir required", nameof(destDir));
@@ -46,6 +52,13 @@ public static void ExtractTo(string zipPath, string destDir)
4652
continue;
4753
}
4854

55+
// Allowlist gate: skip anything that isn't an inert asset type the caller permits.
56+
if (allowedExtensions != null && allowedExtensions.Count > 0
57+
&& !allowedExtensions.Contains(Path.GetExtension(entry.Name).ToLowerInvariant()))
58+
{
59+
continue;
60+
}
61+
4962
string parent = Path.GetDirectoryName(target);
5063
if (!string.IsNullOrEmpty(parent)) Directory.CreateDirectory(parent);
5164

MCPForUnity/Editor/Services/AssetGen/Providers/AssetGenProviders.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,5 +66,13 @@ private static bool IsConfigured(string id)
6666
try { return SecureKeyStore.Current.Has(id); }
6767
catch { return false; }
6868
}
69+
70+
/// <summary>
71+
/// Standard "no key" message: points the user at the Asset Generation tab and the env override.
72+
/// Shared by the asset-gen tools and the job manager so the wording stays in one place.
73+
/// </summary>
74+
public static string MissingKeyMessage(string provider)
75+
=> $"No API key configured for '{provider}'. Add it in the MCP for Unity → Asset Generation tab " +
76+
$"(or set MCPFORUNITY_{(provider ?? string.Empty).ToUpperInvariant()}_API_KEY).";
6977
}
7078
}

0 commit comments

Comments
 (0)