Stop registering colliding and unreachable type globals in JavaScript expressions (#7893)
* fix(javascript): stop registering colliding and unreachable type globals `Engine.RegisterType` exposes a .NET type under `Type.Name`. That name is not always usable, and the type registrations are contributed by several independent handlers whose sets overlap. * `IDictionary<string, string>` and `IDictionary<string, object>` are both named ``IDictionary`2``, so the two registrations claimed the same global and the later one silently won. Neither is reachable from a script: a backtick cannot appear in an identifier. * `byte[]` is named `Byte[]`, which is likewise unreachable. * `DateTime`, `DateTimeOffset`, `TimeSpan`, `Guid` and `LogPersistenceMode` are part of both the common type set and the default workflow variable descriptor set, so each was constructed and assigned twice for every expression evaluation. `RegisterType` now skips types whose name is not usable as a JavaScript identifier, and skips a type that is already registered under that name. Type aliases used by the TypeScript definition endpoint are unaffected — they are maintained by `ITypeAliasRegistry` and are independent of this registration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0179sA2T7HuRfRfSc2JirFik * fix(javascript): leave an already-occupied global name alone `RegisterType` skipped a name only when it already held a `TypeReference` for the same type, so anything else under that name was replaced. That includes a global the host installed through the per-evaluation `configureEngine` callback, `JintOptions.ConfigureEngine` or `JintOptions.RegisterType` — all of which run before the built-in registrations, since those are contributed by handlers of `EvaluatingJavaScript`. Silently overwriting a host global is surprising and the host has no way to win. `RegisterType` now leaves any occupied name alone. That keeps the duplicate suppression the check was written for — registering the same type twice is still a no-op, so the overlapping handlers stop describing the same types through reflection on every evaluation — and additionally makes the host global win. It also agrees with #7895, where the registrations move to engine construction and every host extension point runs after them. Two tests pin the behaviour: a host value set under a built-in type's name survives the built-in registrations, and `RegisterType` installs a `TypeReference` that a second registration leaves untouched. The remark about unusable type names is tightened while here: ``IDictionary`2`` and `Byte[]` can be reached through bracket notation if they are registered, so the reason to skip them is that they cannot be written as identifiers, and that every constructed generic type of the same arity claims the same global. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0179sA2T7HuRfRfSc2JirFik --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Marko Lahma
GitHub
parent
66b9079d3e
commit
58c3c799f8
|
|
@ -15,12 +15,56 @@ public static class EngineExtensions
|
|||
/// <summary>
|
||||
/// Register the specified type <c>T</c> with the engine.
|
||||
/// </summary>
|
||||
public static void RegisterType<T>(this Engine engine) => engine.SetValue(typeof(T).Name, TypeReference.CreateTypeReference(engine, typeof(T)));
|
||||
|
||||
public static void RegisterType<T>(this Engine engine) => engine.RegisterType(typeof(T));
|
||||
|
||||
/// <summary>
|
||||
/// Register the specified type <c>T</c> with the engine.
|
||||
/// Register the specified type with the engine, under its type name.
|
||||
/// </summary>
|
||||
public static void RegisterType(this Engine engine, Type type) => engine.SetValue(type.Name, TypeReference.CreateTypeReference(engine, type));
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Types whose name cannot be written as a JavaScript identifier are skipped. A constructed generic type such
|
||||
/// as <c>IDictionary<string, object></c> is named <c>IDictionary`2</c> and an array type such as
|
||||
/// <c>byte[]</c> is named <c>Byte[]</c>. Registered, those would only be reachable through bracket notation —
|
||||
/// <c>globalThis['Byte[]']</c> — and every constructed generic type of the same arity would claim the same
|
||||
/// global, so the last one registered would silently win.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// A name that is already taken is left alone. Registering the same type twice is therefore a no-op, and a
|
||||
/// global the host installed — through <c>JintOptions.RegisterType</c>, <c>JintOptions.ConfigureEngine</c> or
|
||||
/// the per-evaluation <c>configureEngine</c> callback, all of which run before the built-in registrations —
|
||||
/// is never replaced.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public static void RegisterType(this Engine engine, Type type)
|
||||
{
|
||||
var name = type.Name;
|
||||
|
||||
if (!IsUsableAsIdentifier(name))
|
||||
return;
|
||||
|
||||
// The type registrations are contributed by several independent handlers, whose sets overlap, and they run
|
||||
// after the host has configured the engine. Leaving an occupied name alone does two things: the overlapping
|
||||
// types are no longer described through reflection again for every expression evaluation, and a global that
|
||||
// the host - or JavaScript itself - already put there keeps its value.
|
||||
if (engine.Global.HasOwnProperty(name))
|
||||
return;
|
||||
|
||||
engine.SetValue(name, TypeReference.CreateTypeReference(engine, type));
|
||||
}
|
||||
|
||||
private static bool IsUsableAsIdentifier(string name)
|
||||
{
|
||||
if (string.IsNullOrEmpty(name) || (!char.IsLetter(name[0]) && name[0] != '_' && name[0] != '$'))
|
||||
return false;
|
||||
|
||||
foreach (var c in name)
|
||||
{
|
||||
if (!char.IsLetterOrDigit(c) && c != '_' && c != '$')
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
internal static void SyncVariablesContainer(this Engine engine, IOptions<JintOptions> options, string name, object? value)
|
||||
{
|
||||
|
|
@ -32,4 +76,4 @@ public static class EngineExtensions
|
|||
variablesContainer[name] = ObjectConverterHelper.ProcessVariableValue(engine, value);
|
||||
engine.SetValue("variables", variablesContainer);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,91 @@
|
|||
using Elsa.Expressions.JavaScript.Contracts;
|
||||
using Elsa.Expressions.Models;
|
||||
using Elsa.Extensions;
|
||||
using Elsa.Testing.Shared;
|
||||
using Jint;
|
||||
using Jint.Native;
|
||||
using Jint.Runtime.Interop;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Xunit;
|
||||
using Xunit.Abstractions;
|
||||
|
||||
namespace Elsa.JavaScript.IntegrationTests;
|
||||
|
||||
/// <summary>
|
||||
/// Verifies which .NET types are exposed to JavaScript and under which names.
|
||||
/// </summary>
|
||||
public class TypeRegistrationTests
|
||||
{
|
||||
private readonly IServiceProvider _serviceProvider;
|
||||
private readonly IJavaScriptEvaluator _evaluator;
|
||||
|
||||
public TypeRegistrationTests(ITestOutputHelper testOutputHelper)
|
||||
{
|
||||
_serviceProvider = new TestApplicationBuilder(testOutputHelper).Build();
|
||||
_evaluator = _serviceProvider.GetRequiredService<IJavaScriptEvaluator>();
|
||||
}
|
||||
|
||||
[Theory(DisplayName = "Common .NET types are available under their type name")]
|
||||
[InlineData("DateTime")]
|
||||
[InlineData("DateTimeOffset")]
|
||||
[InlineData("TimeSpan")]
|
||||
[InlineData("Guid")]
|
||||
[InlineData("Random")]
|
||||
[InlineData("LogPersistenceMode")]
|
||||
[InlineData("ExpandoObject")]
|
||||
[InlineData("JsonElement")]
|
||||
[InlineData("JsonNode")]
|
||||
[InlineData("JsonObject")]
|
||||
[InlineData("Stream")]
|
||||
public async Task CommonTypesAreRegistered(string typeName)
|
||||
{
|
||||
Assert.Equal("function", await EvaluateAsync($"return typeof {typeName};"));
|
||||
}
|
||||
|
||||
[Theory(DisplayName = "Types whose name is not a JavaScript identifier are not registered")]
|
||||
[InlineData("IDictionary`2")]
|
||||
[InlineData("Byte[]")]
|
||||
public async Task TypesWithUnusableNamesAreNotRegistered(string globalName)
|
||||
{
|
||||
Assert.Equal("undefined", await EvaluateAsync($"return typeof globalThis['{globalName}'];"));
|
||||
}
|
||||
|
||||
[Fact(DisplayName = "A type registered by the host stays available after the built-in registrations run")]
|
||||
public async Task HostRegisteredTypesSurviveTheBuiltInRegistrations()
|
||||
{
|
||||
Assert.Equal("function", await EvaluateAsync("return typeof Uri;", engine => engine.RegisterType<Uri>()));
|
||||
}
|
||||
|
||||
[Fact(DisplayName = "A global installed by the host wins over the built-in registration of the same name")]
|
||||
public async Task HostGlobalsAreNotOverwrittenByTheBuiltInRegistrations()
|
||||
{
|
||||
// Guid is registered by both ConfigureEngineWithCommonTypes and the default variable descriptor set, and
|
||||
// those handlers run after the configureEngine callback. The host's value has to survive them.
|
||||
Assert.Equal("host-provided", await EvaluateAsync("return Guid;", engine => engine.SetValue("Guid", "host-provided")));
|
||||
}
|
||||
|
||||
[Fact(DisplayName = "A registered type is exposed as a CLR type reference and registering it again is a no-op")]
|
||||
public async Task RegisterTypeInstallsATypeReferenceAndIsIdempotent()
|
||||
{
|
||||
JsValue afterFirstRegistration = JsValue.Undefined;
|
||||
JsValue afterSecondRegistration = JsValue.Undefined;
|
||||
|
||||
await EvaluateAsync("return 'done';", engine =>
|
||||
{
|
||||
engine.RegisterType<Uri>();
|
||||
afterFirstRegistration = engine.GetValue("Uri");
|
||||
engine.RegisterType<Uri>();
|
||||
afterSecondRegistration = engine.GetValue("Uri");
|
||||
});
|
||||
|
||||
var typeReference = Assert.IsType<TypeReference>(afterFirstRegistration);
|
||||
Assert.Equal(typeof(Uri), typeReference.ReferenceType);
|
||||
Assert.Same(afterFirstRegistration, afterSecondRegistration);
|
||||
}
|
||||
|
||||
private async Task<string?> EvaluateAsync(string script, Action<Engine>? configureEngine = null)
|
||||
{
|
||||
var context = new ExpressionExecutionContext(_serviceProvider, new());
|
||||
return (string?)await _evaluator.EvaluateAsync(script, typeof(string), context, configureEngine: configureEngine);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue