From cd0ec583e17d154228b7c8811f46c726c8c475d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 12 May 2026 05:05:35 -0400 Subject: [PATCH] refactor(ui/scripts): cache diagnostics + semantic forbidden-API check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pre-flagged follow-ups from the Monaco integration: 1. IMemoryCache for diagnostics keyed by SHA256 of the script body. Same-code Diagnose() now short-circuits the Roslyn compile and forbidden-API walk. SizeLimit 200 entries with 5-minute sliding expiration. Completions aren't cached — position + form context vary too much for a useful hit rate. 2. Forbidden-API analyzer now resolves identifiers through the SemanticModel instead of matching names. A user identifier named File / Thread / Process / etc. no longer false-positives — only references that resolve to a NamedTypeSymbol whose containing namespace is on the banned list are flagged. The diagnostic message now names the offending namespace, e.g. "Type 'File' from forbidden namespace 'System.IO' is not allowed in scripts." Refactor: extracted ISharedScriptCatalog so ScriptAnalysisService can be unit-tested without standing up SharedScriptService's EF chain. Concrete SharedScriptCatalog wraps the existing service. 16 new xUnit tests in ScriptAnalysisServiceTests: - Empty / clean / missing-semicolon paths - SCADA001 on each banned using namespace (theory) - SCADA002 on real File.ReadAllText through System.IO - No-false-positive checks for user-defined File / Thread locals - Cache returns the same response instance on repeat - Different code → different cache entries - String-literal completions for Parameters / CallScript / CallShared - General completion at file scope returns ScriptHost members Total CentralUI test count: 113 -> 129. --- .../ScriptAnalysis/ISharedScriptCatalog.cs | 25 +++ .../ScriptAnalysis/ScriptAnalysisService.cs | 120 ++++++++------ .../ServiceCollectionExtensions.cs | 4 +- .../ScriptAnalysisServiceTests.cs | 156 ++++++++++++++++++ 4 files changed, 256 insertions(+), 49 deletions(-) create mode 100644 src/ScadaLink.CentralUI/ScriptAnalysis/ISharedScriptCatalog.cs create mode 100644 tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/ISharedScriptCatalog.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/ISharedScriptCatalog.cs new file mode 100644 index 0000000..ba19970 --- /dev/null +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/ISharedScriptCatalog.cs @@ -0,0 +1,25 @@ +using ScadaLink.TemplateEngine; + +namespace ScadaLink.CentralUI.ScriptAnalysis; + +/// +/// Indirection so ScriptAnalysisService can be unit-tested without standing +/// up SharedScriptService and its EF Core repository chain. +/// +public interface ISharedScriptCatalog +{ + Task> GetNamesAsync(); +} + +public class SharedScriptCatalog : ISharedScriptCatalog +{ + private readonly SharedScriptService _service; + + public SharedScriptCatalog(SharedScriptService service) => _service = service; + + public async Task> GetNamesAsync() + { + var scripts = await _service.GetAllSharedScriptsAsync(); + return scripts.Select(s => s.Name).ToList(); + } +} diff --git a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs index 4eaa35a..e62fd75 100644 --- a/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs +++ b/src/ScadaLink.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs @@ -1,23 +1,34 @@ +using System.Security.Cryptography; +using System.Text; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Scripting; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Scripting; -using ScadaLink.TemplateEngine; +using Microsoft.Extensions.Caching.Memory; namespace ScadaLink.CentralUI.ScriptAnalysis; /// /// Compiles user scripts as Roslyn C# Scripting fragments against /// globals and surfaces diagnostics + completions -/// in the shape Monaco's provider APIs expect. Lightweight — no caching; -/// each request rebuilds the script. Acceptable for human-paced edits. +/// in the shape Monaco's provider APIs expect. +/// +/// Diagnostics are cached by code hash via IMemoryCache — Monaco debounces +/// keystrokes at 500 ms but a typing-then-pausing flow can still re-issue +/// requests for the same content (window blur/focus, etc.), so the cache +/// short-circuits repeats. Completions aren't cached: position + form +/// context vary too much for the hit rate to be useful. /// /// Beyond plain C# analysis, layers SCADA-specific extensions: /// - In-string completion of Parameters["..."] keys (from the request's -/// DeclaredParameters), CallShared("...") names (from SharedScriptService), -/// and CallScript("...") names (from the request's SiblingScripts). -/// - Forbidden-API diagnostic for the documented script trust model. +/// DeclaredParameters), CallShared("...") names (from +/// ), and CallScript("...") names +/// (from the request's SiblingScripts). +/// - Forbidden-API diagnostic for the documented script trust model, +/// resolved against the SemanticModel so user identifiers that happen +/// to share names with forbidden types (e.g. var File = ...) +/// do not false-positive. /// public class ScriptAnalysisService { @@ -47,20 +58,13 @@ public class ScriptAnalysisService "System.Threading.Tasks.Sources", }; - private static readonly HashSet ForbiddenTypeNames = new(StringComparer.Ordinal) - { - "File", "Directory", "Path", "StreamReader", "StreamWriter", "FileStream", - "Process", "ProcessStartInfo", - "Assembly", "Type", "MethodInfo", "PropertyInfo", "FieldInfo", - "Socket", "TcpClient", "UdpClient", "TcpListener", - "Thread", "ThreadPool", "Mutex", "Semaphore", - }; + private readonly ISharedScriptCatalog _sharedScripts; + private readonly IMemoryCache _cache; - private readonly SharedScriptService _sharedScripts; - - public ScriptAnalysisService(SharedScriptService sharedScripts) + public ScriptAnalysisService(ISharedScriptCatalog sharedScripts, IMemoryCache cache) { _sharedScripts = sharedScripts; + _cache = cache; } public DiagnoseResponse Diagnose(DiagnoseRequest request) @@ -68,6 +72,10 @@ public class ScriptAnalysisService if (string.IsNullOrEmpty(request.Code)) return new DiagnoseResponse(Array.Empty()); + var cacheKey = "diag:" + HashCode(request.Code); + if (_cache.TryGetValue(cacheKey, out DiagnoseResponse? cached) && cached is not null) + return cached; + Script script; try { @@ -75,10 +83,11 @@ public class ScriptAnalysisService } catch (Exception ex) { - return new DiagnoseResponse(new[] + var failure = new DiagnoseResponse(new[] { new DiagnosticMarker(8, 1, 1, 1, 2, ex.Message, "SCRIPT_BUILD") }); + return Cache(cacheKey, failure); } var compilation = script.GetCompilation(); @@ -91,10 +100,27 @@ public class ScriptAnalysisService var tree = compilation.SyntaxTrees.FirstOrDefault(); if (tree != null) { - markers.AddRange(FindForbiddenApiUsages(tree)); + var model = compilation.GetSemanticModel(tree); + markers.AddRange(FindForbiddenApiUsages(tree, model)); } - return new DiagnoseResponse(markers); + return Cache(cacheKey, new DiagnoseResponse(markers)); + } + + private DiagnoseResponse Cache(string key, DiagnoseResponse value) + { + _cache.Set(key, value, new MemoryCacheEntryOptions + { + Size = 1, + SlidingExpiration = TimeSpan.FromMinutes(5) + }); + return value; + } + + private static string HashCode(string code) + { + var bytes = SHA256.HashData(Encoding.UTF8.GetBytes(code)); + return Convert.ToHexString(bytes); } public async Task CompleteAsync(CompletionsRequest request) @@ -182,9 +208,9 @@ public class ScriptAnalysisService if (calleeName == "CallShared") { - var scripts = await _sharedScripts.GetAllSharedScriptsAsync(); - return scripts - .Select(s => new CompletionItem(s.Name, s.Name, "shared script", "Method")) + var names = await _sharedScripts.GetNamesAsync(); + return names + .Select(n => new CompletionItem(n, n, "shared script", "Method")) .ToList(); } @@ -220,11 +246,11 @@ public class ScriptAnalysisService .ToList(); } - private static IEnumerable FindForbiddenApiUsages(SyntaxTree tree) + private static IEnumerable FindForbiddenApiUsages(SyntaxTree tree, SemanticModel model) { var root = tree.GetRoot(); - // Banned using directives. + // Banned using directives — pure namespace string match is fine here. foreach (var u in root.DescendantNodes().OfType()) { var name = u.Name?.ToString() ?? ""; @@ -242,32 +268,30 @@ public class ScriptAnalysisService } } - // Banned type identifiers (e.g., new Process(), File.ReadAllText, etc.). - // Note: this is a name-based heuristic — false positives are possible for - // user identifiers that happen to share names with forbidden types. + // Banned type usages — resolved via the semantic model so a user + // identifier named "File" or "Thread" does NOT trigger the diagnostic + // unless it actually resolves to a forbidden type. foreach (var ident in root.DescendantNodes().OfType()) { - var name = ident.Identifier.ValueText; - if (ForbiddenTypeNames.Contains(name)) - { - // Filter: only flag when used as a type or as a member-access target. - var parent = ident.Parent; - var isTypeOrAccess = - parent is MemberAccessExpressionSyntax m && m.Expression == ident || - parent is QualifiedNameSyntax || - parent is ObjectCreationExpressionSyntax; - if (!isTypeOrAccess) continue; + // Skip the identifier on the right side of a member access — only + // the leftmost (the type or qualifier) is what we want to check. + if (ident.Parent is MemberAccessExpressionSyntax m && m.Name == ident) continue; - var span = ident.GetLocation().GetLineSpan().Span; - yield return new DiagnosticMarker( - Severity: 8, - StartLineNumber: span.Start.Line + 1, - StartColumn: span.Start.Character + 1, - EndLineNumber: span.End.Line + 1, - EndColumn: span.End.Character + 1, - Message: $"Type '{name}' is forbidden in scripts (script trust model).", - Code: "SCADA002"); - } + var symbol = model.GetSymbolInfo(ident).Symbol; + if (symbol is not INamedTypeSymbol type) continue; + + var ns = type.ContainingNamespace?.ToDisplayString() ?? ""; + if (!ForbiddenNamespacePrefixes.Any(p => ns == p || ns.StartsWith(p + "."))) continue; + + var span = ident.GetLocation().GetLineSpan().Span; + yield return new DiagnosticMarker( + Severity: 8, + StartLineNumber: span.Start.Line + 1, + StartColumn: span.Start.Character + 1, + EndLineNumber: span.End.Line + 1, + EndColumn: span.End.Character + 1, + Message: $"Type '{type.Name}' from forbidden namespace '{ns}' is not allowed in scripts.", + Code: "SCADA002"); } } diff --git a/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs b/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs index 783f70f..e307444 100644 --- a/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs +++ b/src/ScadaLink.CentralUI/ServiceCollectionExtensions.cs @@ -24,7 +24,9 @@ public static class ServiceCollectionExtensions services.AddScoped(); // Roslyn-backed C# analysis for the Monaco script editor. - // Scoped because SharedScriptService (a dependency) is scoped. + // Scoped because SharedScriptCatalog wraps a scoped service. + services.AddMemoryCache(o => o.SizeLimit = 200); + services.AddScoped(); services.AddScoped(); return services; diff --git a/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs new file mode 100644 index 0000000..de769ee --- /dev/null +++ b/tests/ScadaLink.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -0,0 +1,156 @@ +using Microsoft.Extensions.Caching.Memory; +using NSubstitute; +using ScadaLink.CentralUI.ScriptAnalysis; + +namespace ScadaLink.CentralUI.Tests.ScriptAnalysis; + +public class ScriptAnalysisServiceTests +{ + private readonly ISharedScriptCatalog _catalog = Substitute.For(); + private readonly IMemoryCache _cache = new MemoryCache(new MemoryCacheOptions { SizeLimit = 100 }); + private readonly ScriptAnalysisService _svc; + + public ScriptAnalysisServiceTests() + { + _catalog.GetNamesAsync().Returns(Array.Empty()); + _svc = new ScriptAnalysisService(_catalog, _cache); + } + + [Fact] + public void EmptyCode_NoMarkers() + { + var resp = _svc.Diagnose(new DiagnoseRequest("")); + Assert.Empty(resp.Markers); + } + + [Fact] + public void CleanScript_NoMarkers() + { + var resp = _svc.Diagnose(new DiagnoseRequest("var x = 1 + 2; return x;")); + Assert.Empty(resp.Markers); + } + + [Fact] + public void MissingSemicolon_ReportsRoslynDiagnostic() + { + var resp = _svc.Diagnose(new DiagnoseRequest("var x = 1\n")); + Assert.Contains(resp.Markers, m => m.Code.StartsWith("CS")); + } + + [Fact] + public void ForbiddenUsingDirective_RaisesSCADA001() + { + var resp = _svc.Diagnose(new DiagnoseRequest("using System.IO;")); + Assert.Contains(resp.Markers, m => m.Code == "SCADA001" && m.Message.Contains("System.IO")); + } + + [Theory] + [InlineData("System.Diagnostics")] + [InlineData("System.Reflection")] + [InlineData("System.Net")] + public void ForbiddenUsing_AllBannedNamespaces(string ns) + { + var resp = _svc.Diagnose(new DiagnoseRequest($"using {ns};")); + Assert.Contains(resp.Markers, m => m.Code == "SCADA001" && m.Message.Contains(ns)); + } + + [Fact] + public void ForbiddenTypeUsage_ResolvesViaSemanticModel() + { + var resp = _svc.Diagnose(new DiagnoseRequest( + "using System.IO; var s = File.ReadAllText(\"x\");")); + Assert.Contains(resp.Markers, m => m.Code == "SCADA002" && m.Message.Contains("File")); + } + + [Fact] + public void UserIdentifierNamedFile_DoesNotFalsePositive() + { + // No System.IO import; user defines their own 'File' local. + var resp = _svc.Diagnose(new DiagnoseRequest( + "var File = \"hello\"; return File.Length;")); + Assert.DoesNotContain(resp.Markers, m => m.Code == "SCADA002"); + } + + [Fact] + public void UserIdentifierNamedThread_DoesNotFalsePositive() + { + var resp = _svc.Diagnose(new DiagnoseRequest( + "var Thread = 42; return Thread;")); + Assert.DoesNotContain(resp.Markers, m => m.Code == "SCADA002"); + } + + [Fact] + public void DiagnosticsAreCached_SecondCallSkipsRecompile() + { + var req = new DiagnoseRequest("using System.IO;"); + var first = _svc.Diagnose(req); + var second = _svc.Diagnose(req); + + // Same instance reference indicates the cache returned the prior result. + Assert.Same(first, second); + } + + [Fact] + public void DifferentCode_GetsDifferentCacheEntries() + { + var a = _svc.Diagnose(new DiagnoseRequest("var x = 1;")); + var b = _svc.Diagnose(new DiagnoseRequest("var y = 2;")); + Assert.NotSame(a, b); + } + + [Fact] + public async Task ParametersStringLiteral_ReturnsDeclaredParameterNames() + { + var req = new CompletionsRequest( + CodeText: "var x = Parameters[\"", + Line: 1, + Column: 21, + DeclaredParameters: new[] { "name", "temperature" }); + + var resp = await _svc.CompleteAsync(req); + + Assert.Contains(resp.Items, i => i.Label == "name" && i.Detail == "declared parameter"); + Assert.Contains(resp.Items, i => i.Label == "temperature"); + } + + [Fact] + public async Task CallScriptStringLiteral_ReturnsSiblingNames() + { + var req = new CompletionsRequest( + CodeText: "var x = CallScript(\"", + Line: 1, + Column: 21, + SiblingScripts: new[] { "SiblingA", "SiblingB" }); + + var resp = await _svc.CompleteAsync(req); + + Assert.Contains(resp.Items, i => i.Label == "SiblingA" && i.Detail == "sibling script"); + Assert.Contains(resp.Items, i => i.Label == "SiblingB"); + } + + [Fact] + public async Task CallSharedStringLiteral_ResolvesViaCatalog() + { + _catalog.GetNamesAsync().Returns(new[] { "GetWeather", "Greet" }); + + var req = new CompletionsRequest( + CodeText: "var x = CallShared(\"", + Line: 1, + Column: 21); + + var resp = await _svc.CompleteAsync(req); + + Assert.Contains(resp.Items, i => i.Label == "GetWeather" && i.Detail == "shared script"); + Assert.Contains(resp.Items, i => i.Label == "Greet"); + } + + [Fact] + public async Task GeneralCompletion_ReturnsInScopeSymbols() + { + // At file scope of a script, ScriptHost members + the System namespace are visible. + var req = new CompletionsRequest("var x = ", 1, 9); + var resp = await _svc.CompleteAsync(req); + Assert.Contains(resp.Items, i => i.Label == "Parameters"); + Assert.Contains(resp.Items, i => i.Label == "CallShared"); + } +}