Interop: an index outside a wrapped collection is refused, not handed to the collection

Jint.Tests.PublicInterface/HostCollectionIndexBoundsTests.cs

@@ -0,0 +1,203 @@
+#nullable enable
+
+using System.Collections;
+using System.Globalization;
+using Jint.Native;
+using Jint.Runtime;
+
+namespace Jint.Tests.PublicInterface;
+
+/// <summary>
+/// A host collection that has a count and an indexer of its own, and none of the three interfaces that would
+/// give it an array-like <em>view</em>. It is wrapped plainly, so every index-shaped key resolves the
+/// reflected indexer — which takes whatever index it parsed out of the key straight to the collection.
+/// </summary>
+/// <remarks>
+/// <para>
+/// #3384 gave an <c>ArrayLikeWrapper</c> ownership of every index-shaped key, so an out-of-range write is
+/// refused rather than handed over. The refusal lives in the array-like view, and this shape does not get one:
+/// a plain <see cref="Jint.Runtime.Interop.ObjectWrapper"/> still went to the indexer and let the collection's
+/// own <see cref="ArgumentOutOfRangeException"/> out of <c>Evaluate</c>, where neither a script
+/// <c>try</c>/<c>catch</c> nor a host <c>catch (JavaScriptException)</c> can see it (#3422).
+/// </para>
+/// <para>
+/// The guard cannot be the array-like one, because a plain wrapper's indexer lane also serves
+/// <c>Dictionary&lt;int, string&gt;</c>, where <c>d[99] = "x"</c> is a legitimate add. Both halves are pinned
+/// here.
+/// </para>
+/// </remarks>
+public class HostCollectionIndexBoundsTests
+{
+    /// <summary>Keys naming a position a three-element window cannot address, in both spellings.</summary>
+    public static IEnumerable<string> UnaddressableIndices => ["3", "'3'", "10", "-1", "'-1'", "'08'", "'+3'"];
+
+    [TestCaseSource(nameof(UnaddressableIndices))]
+    public void ReadingAnIndexTheCollectionCannotAddressAnswersUndefined(string key)
+    {
+        var engine = CreateEngine(out _);
+
+        engine.Evaluate($"x[{key}]").Should().Be(JsValue.Undefined,
+            "there is no element at {0}, and a hole reads undefined rather than raising the collection's own exception", key);
+    }
+
+    [TestCaseSource(nameof(UnaddressableIndices))]
+    public void WritingAnIndexTheCollectionCannotAddressIsTheOrdinarySetRefusal(string key)
+    {
+        var engine = CreateEngine(out var window);
+
+        engine.Execute($"x[{key}] = 9");
+        engine.Evaluate($"Reflect.set(x, {key}, 9)").AsBoolean().Should().BeFalse();
+
+        var thrown = Caught.Exception(() => engine.Execute($"'use strict'; x[{key}] = 9;"));
+        thrown.Should().BeOfType<JavaScriptException>("a refused [[Set]] in strict mode is a TypeError");
+        ((JavaScriptException) thrown!).Error.Get("name").AsString().Should().Be("TypeError");
+
+        window.Should().Equal(new[] { 1, 2, 3 }, "nothing reached the collection");
+    }
+
+    [TestCaseSource(nameof(UnaddressableIndices))]
+    public void EveryExistenceLaneAgreesThatSuchAnIndexIsAbsent(string key)
+    {
+        var engine = CreateEngine(out _);
+
+        engine.Evaluate($"[({key} in x), x.hasOwnProperty({key}), x.propertyIsEnumerable({key}), Object.getOwnPropertyDescriptor(x, {key}) !== undefined].join(',')")
+            .AsString().Should().Be("false,false,false,false",
+                "{0} is not a position of the collection, so no lane may report it as one", key);
+    }
+
+    [TestCaseSource(nameof(UnaddressableIndices))]
+    public void DeletingSuchAnIndexSucceedsWithoutTouchingTheCollection(string key)
+    {
+        var engine = CreateEngine(out var window);
+
+        engine.Evaluate($"delete x[{key}]").AsBoolean().Should().BeTrue("OrdinaryDelete returns true for a property that is not there");
+        window.Should().Equal(1, 2, 3);
+    }
+
+    /// <summary>
+    /// The contrast that says the guard is a bound and not a ban on integer keys: every position the
+    /// collection does have still reads and writes through its own indexer.
+    /// </summary>
+    [TestCase("0", 1d)]
+    [TestCase("'0'", 1d)]
+    [TestCase("2", 3d)]
+    [TestCase("'2'", 3d)]
+    public void APositionTheCollectionHasIsUnaffected(string key, double expected)
+    {
+        var engine = CreateEngine(out var window);
+
+        engine.Evaluate($"x[{key}]").AsNumber().Should().Be(expected);
+        engine.Evaluate($"{key} in x").AsBoolean().Should().BeTrue();
+        engine.Evaluate($"x.hasOwnProperty({key})").AsBoolean().Should().BeTrue();
+
+        engine.Execute($"x[{key}] = 9");
+        window[int.Parse(key.Trim('\''), CultureInfo.InvariantCulture)].Should().Be(9);
+    }
+
+    /// <summary>Named CLR members of the same target keep resolving; only index-shaped keys are the view's.</summary>
+    [Test]
+    public void NamedMembersAreUnaffected()
+    {
+        var engine = CreateEngine(out _);
+
+        engine.Evaluate("x.Count").AsNumber().Should().Be(3);
+        engine.Evaluate("x.length").AsNumber().Should().Be(3);
+        engine.Evaluate("Array.prototype.join.call(x, '-')").AsString().Should().Be("1-2-3");
+    }
+
+    /// <summary>
+    /// The other half of the trade-off. A wrapper's indexer lane also serves a dictionary, where a key outside
+    /// what it currently holds is a legitimate add and refusing it would be the bug. `IsArrayLike` is what
+    /// separates the two, and it is `false` for a dictionary.
+    /// </summary>
+    [Test]
+    public void AnIntegerKeyedDictionaryStillAdds()
+    {
+        var engine = new Engine(options =>
+        {
+            options.AllowClr();
+            options.Interop.AllowWrite = true;
+        });
+        var dictionary = new Dictionary<int, string> { [1] = "one" };
+        engine.SetValue("x", dictionary);
+
+        engine.Execute("x[99] = 'ninety-nine'");
+
+        dictionary.Should().ContainKey(99).WhoseValue.Should().Be("ninety-nine");
+        engine.Evaluate("x[99]").AsString().Should().Be("ninety-nine");
+        engine.Evaluate("x[1]").AsString().Should().Be("one");
+    }
+
+    /// <summary>
+    /// A string-keyed indexer on an array-like target is not the collection's positions, and the check reads
+    /// the resolved accessor's index parameter rather than guessing from the key, so it stays out of the way.
+    /// <c>headers["3"]</c> is the header named <c>3</c>, whatever the count happens to be.
+    /// </summary>
+    [Test]
+    public void AStringKeyedIndexerOnAnArrayLikeTargetStillAnswers()
+    {
+        var engine = new Engine(options =>
+        {
+            options.AllowClr();
+            options.Interop.AllowWrite = true;
+        });
+        engine.SetValue("x", new NamedValues { ["3"] = "the third", ["etag"] = "abc" });
+
+        engine.Evaluate("x['3']").AsString().Should().Be("the third");
+        engine.Evaluate("x['etag']").AsString().Should().Be("abc");
+    }
+
+    private static Engine CreateEngine(out Window window)
+    {
+        window = new Window(1, 2, 3);
+        var engine = new Engine(options =>
+        {
+            options.AllowClr();
+            options.Interop.AllowWrite = true;
+        });
+        engine.SetValue("x", window);
+        return engine;
+    }
+
+    /// <summary>
+    /// Bounded and indexed, and neither an <see cref="IList{T}"/>, an <see cref="IReadOnlyList{T}"/> nor a
+    /// non-generic <see cref="IList"/> — so no array-like view is built for it and every part of that is
+    /// load-bearing. <see cref="IReadOnlyCollection{T}"/> is what gives it a <c>Count</c>, and the declared
+    /// indexer is what the reflected member lane resolves for an index-shaped key.
+    /// </summary>
+    public sealed class Window : IReadOnlyCollection<int>
+    {
+        private readonly List<int> _items;
+
+        public Window(params int[] items) => _items = [.. items];
+
+        public int this[int index] { get => _items[index]; set => _items[index] = value; }
+
+        public int Count => _items.Count;
+
+        public IEnumerator<int> GetEnumerator() => _items.GetEnumerator();
+
+        IEnumerator IEnumerable.GetEnumerator() => _items.GetEnumerator();
+    }
+
+    /// <summary>
+    /// An array-like target whose indexer is keyed by <see cref="string"/>, the shape a
+    /// <c>NameValueCollection</c> has.
+    /// </summary>
+    public sealed class NamedValues : IReadOnlyCollection<string>
+    {
+        private readonly Dictionary<string, string> _values = new(StringComparer.Ordinal);
+
+        public string this[string name]
+        {
+            get => _values.TryGetValue(name, out var value) ? value : string.Empty;
+            set => _values[name] = value;
+        }
+
+        public int Count => _values.Count;
+
+        public IEnumerator<string> GetEnumerator() => _values.Values.GetEnumerator();
+
+        IEnumerator IEnumerable.GetEnumerator() => _values.Values.GetEnumerator();
+    }
+}

Jint/Runtime/Interop/AGENTS.md

@@ -65,6 +65,7 @@ rest of the list is split across the files indexed from the repository-root [`AG
 - **A wrapper's prototype comes from the engine's *running* realm, so building it is realm-scoped work.** `JsValue.FromObject` reaches `ObjectInstance`'s base constructor, `TypeReference.CreateTypeReference` reaches `TypeReferencePrototype`, `DelegateWrapper` reads it outright — all three take `engine.Realm.Intrinsics`, which is `ExecutionContext.Realm` and therefore whichever realm happens to be current at the call. That is right for `Engine.SetValue`, where the realm being written *is* the current one, and wrong for anything projecting into a second realm: `ShadowRealm.SetValue` converted against the principal realm and installed on the shadow realm's global, so a host object handed to a realm was not `instanceof Object` inside it ([#3325](https://github.com/sebastienros/jint/issues/3325)). Jint has exactly one realm-scoped construction path — push that realm's `ExecutionContext` for the duration (`ShadowRealm.EnterRealm`, and `ShadowRealmImportValue` before it) — and no lighter one; `Engine._realmInConstruction` is not it, being neither nestable nor exception-safe. Three host-facing base constructors deliberately do **not** follow the running realm: `ClrFunction(Engine, …)`, `HostFunction` and `Constructor(Engine, string)` pin `engine._originalIntrinsics`, because a function a host builds against an `Engine` belongs to the realm the surrounding script can reach whatever was running when it was built ([#2893](https://github.com/sebastienros/jint/pull/2893)). The two rules do not conflict — one is about a realm named by the caller, the other about a realm that merely happened to be current — but a member built through those constructors *inside* a wrapper (its `toJSON`, its `Symbol.dispose`) still lands in the principal realm, which realm-scoping the wrapper does not change.
 - **Array-like is not indexable, and `TypeDescriptor.IsArrayLike` is the weaker of the two.** It means the target has a `Count`, nothing more: `ICollection`, `ICollection<T>` and `IReadOnlyCollection<T>` are count-and-copy contracts with no index in them, so `Queue<T>`, `Stack<T>`, `LinkedList<T>`, `SortedSet<T>` and `HashSet<T>` are all array-like with no element at index 0. Any lane that *reads by index* must gate on `ObjectWrapper.HasIndexedElements` instead — `ArrayOperations.For` gated on array-likeness and a bare `ICollection` and handed `IndexWrappedOperations` a target it then hard-cast to `IList`, which is [#3302](https://github.com/sebastienros/jint/issues/3302): a raw `InvalidCastException` out of `Evaluate` for every `Array.prototype` generic over a `Queue<T>`. Falling through to `ObjectOperations` is not a consolation prize — it asks the *object*, so a host collection that really does have an integer indexer still gets its elements through the reflected accessor, and only a genuinely index-less one reads `undefined` per index, which is what an array-like with no index properties means. The same rule governs `ObjectWrapper.HasOriginalIterator`: a wrapper's `Symbol.iterator` is never the array iterator (it enumerates the CLR target), so the index-reading fast path it enables for array destructuring may only stand in for `GetIterator` where index reads reproduce what enumeration yields — which is exactly `HasIndexedElements`.
 - **Overload scoring's last rule is the converter's own answer, and it has to be.** `InteropHelper.CalculateMethodParameterScore` rates an argument against a parameter with a dozen structural rules, and everything they did not recognize scored a blanket 100 — "will rarely succeed". `FindBestMatch` discards only a *negative* score, so 100 is a match, and a candidate the argument can never bind to is the *best* one whenever it is the only one. That is survivable where the caller retries — `MethodInfoFunction.TryCall` asks `converter.TryConvert` per candidate and moves on when it declines — and not where the caller takes the first match and stops, which `JintBinaryExpression.TryOperatorOverloading` and `TypeReference`'s constructor selection both do: `'s' + v` selected `op_Addition(T, T)` and died converting the string instead of concatenating, for any host type whose only `+` is `(T, T)` ([#3407](https://github.com/sebastienros/jint/issues/3407)). The last rule now asks the installed `ClrTypeConverter` — the very one `MethodDescriptor.Call` will use — so the score cannot claim a conversion the call then fails to perform, and only what it *confirms* keeps the 100. Three things follow. **What the 100 was protecting is three shapes no structural rule can see**: a JS function to a delegate parameter, an enum parameter given a number outside its defined members, and a conversion operator declared on the **target** type — the operator scan above it reads the *argument's* type only, while `DefaultTypeConverter.TryCastWithOperators` reads both. Each is pinned in `Jint.Tests.PublicInterface/HostOverloadScoringTests.cs`, and deleting the probe fails exactly those three and nothing else in the suite. A parameter type still carrying **open** type parameters (`T`, `Func<T, bool>`) is the one thing the converter cannot be asked — the closed type does not exist until `MethodInfoFunction.ResolveMethod` builds it, and handing an open one over is an `ArgumentException` rather than an answer — so those keep the undecided 100. And the probe *performs* the conversion rather than predicting it, so a user-defined `op_Implicit` on a candidate that is then selected runs twice; the `CanChangeType` rule above it already converts speculatively, so that is this function's established cost rather than a new one, but it is why nothing here may assume a conversion happens once.
+- **An index-shaped key on a wrapped bounded collection is the wrapper's to answer, in every lane.** The reflected indexer parses an index out of whatever key it is handed and takes it to the collection, so an out-of-range `x[3] = 9` was the CLR's own `ArgumentOutOfRangeException` out of `Evaluate` — invisible to a script `try`/`catch` and to a host `catch (JavaScriptException)` alike. `ArrayLikeWrapper` owns those keys now ([#3384](https://github.com/sebastienros/jint/issues/3384)), *including* the descriptor lane ([#3423](https://github.com/sebastienros/jint/issues/3423)): `Get`, `Set`, `HasProperty`, `Delete`, `GetOwnProperty`, `ProbeOwnProperty`, `DefineOwnProperty` and both key enumerations all answer from `Length`, and a lane added later that does not is a lane where `in` and `hasOwnProperty` contradict each other — which they may not, `OrdinaryHasProperty` being defined in terms of `[[GetOwnProperty]]`. A **plain** `ObjectWrapper` over a bounded target has no view and reads the target's own count instead ([#3422](https://github.com/sebastienros/jint/issues/3422)); that check is deliberately conditioned on two facts at once, because the same lane serves `Dictionary<int, string>`, where `d[99] = "x"` is a legitimate add, and a string-keyed indexer on a collection, where `x["3"]` names a key rather than a position. `ObjectWrapper.ClassifyElementKey` is the one definition of "index-shaped key" and lives on the base class for that reason; a second copy is the thing that would drift.
 
 ### `[JsAccessible]`: the generated lane, and why it is equivalent rather than merely fast
 

Jint/Runtime/Interop/ObjectWrapper.Specialized.cs

@@ -122,96 +122,6 @@ protected ArrayLikeWrapper(
     // type descriptor does not recognize as integer-indexed
     internal sealed override bool HasIndexedElements => true;
 
-    /// <summary>
-    /// What a property key names on this view. Every element lane below is keyed on the <em>index</em>
-    /// rather than on how script spelled it: <c>x[3]</c> and <c>x["3"]</c> are one property key, because
-    /// <see href="https://tc39.es/ecma262/#sec-topropertykey">ToPropertyKey</see> turns both into the
-    /// String <c>"3"</c>. Answering the two spellings from different lanes is answering one question
-    /// twice, and the lane that used to serve the string spelling was the reflected indexer, which reaches
-    /// the collection with whatever index it parses out of the key (#3384).
-    /// </summary>
-    private enum ElementKey
-    {
-        /// <summary>Not a position of this view: a member name, a symbol, a fractional number, or any string key of a dictionary-shaped target.</summary>
-        None,
-
-        /// <summary>A position the target could address. May be at or past <see cref="Length"/>.</summary>
-        Position,
-
-        /// <summary>Index-shaped but never a position of this view: negative, non-canonical (<c>"08"</c>, <c>"+3"</c>), or past what the target can address.</summary>
-        OutOfBand,
-    }
-
-    /// <summary>
-    /// The highest position an element lane will address, one below <see cref="int.MaxValue"/> so that
-    /// growing to <c>index + 1</c> cannot overflow.
-    /// </summary>
-    private const int MaxPosition = int.MaxValue - 1;
-
-    private ElementKey ClassifyElementKey(JsValue property, out int index)
-    {
-        index = 0;
-
-        if (property is JsNumber number)
-        {
-            var value = number._value;
-            if (!TypeConverter.IsIntegralNumber(value))
-            {
-                return ElementKey.None;
-            }
-
-            if (value < 0 || value > MaxPosition)
-            {
-                return ElementKey.OutOfBand;
-            }
-
-            index = (int) value;
-            return ElementKey.Position;
-        }
-
-        // a symbol names no position, and a dictionary-shaped target (e.g. Newtonsoft's JObject: both
-        // IDictionary<string,_> and IList<_>) answers a string key from its own keys rather than by index
-        if (property is not JsString jsString || _typeDescriptor.IsDictionary)
-        {
-            return ElementKey.None;
-        }
-
-        var member = jsString.ToString();
-        var parsed = ArrayInstance.ParseArrayIndex(member);
-        if (parsed != uint.MaxValue)
-        {
-            if (parsed > MaxPosition)
-            {
-                return ElementKey.OutOfBand;
-            }
-
-            index = (int) parsed;
-            return ElementKey.Position;
-        }
-
-        // One character rules out every ordinary member name that reaches this view — "length", "Count",
-        // "push" — which would otherwise pay a whole long.TryParse only to be rejected by it.
-        if (member.Length == 0 || !IsIntegerShapedStart(member[0]))
-        {
-            return ElementKey.None;
-        }
-
-        // an integer-shaped but non-canonical key ("-1", "08") is not a position of the view either, and
-        // must not fall through to the reflected indexer, which parses one out of it and reaches the
-        // collection with an index the collection rejects
-        return long.TryParse(member, NumberStyles.Integer, CultureInfo.InvariantCulture, out _)
-            ? ElementKey.OutOfBand
-            : ElementKey.None;
-    }
-
-    /// <summary>
-    /// Whether <paramref name="c"/> can begin something <see cref="NumberStyles.Integer"/> parses: a digit,
-    /// a sign, or the leading white space it tolerates.
-    /// </summary>
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    private static bool IsIntegerShapedStart(char c)
-        => (uint) (c - '0') <= 9 || c == '-' || c == '+' || char.IsWhiteSpace(c);
-
     /// <summary>
     /// Whether <paramref name="property"/> spells a position this view does not have — index-shaped, and
     /// either outside <c>[0, Length)</c> or never addressable at all (negative, non-canonical, past what the