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"); + } +}