Repository navigation
Disable projected CLR writes by default - #3054
Conversation
lahma
left a comment
There was a problem hiding this comment.
Reviewed as the incremental diff against sebros/stack-clr-containment-medium. The mechanics are thorough and the bypass-chasing shows: the borrowed/species array operations now funnel length and element mutation through the authority-checked Set paths instead of touching the CLR list directly, frozen-wrapper deletes answer honestly per key kind, the CreateDataPropertyOrThrow fast path respects non-extensibility, and ReflectionDescriptor.DoSet fails loudly with a TypeError — which matters enormously for migration, because a silently-ignored write would have embedders chasing ghost bugs for weeks. AttachOwner wiring so a frozen wrapper's members refuse writes is a nice closure of the detached-descriptor route. Approving the change itself.
The caveat, stated as plainly as I can: flipping Interop.AllowWrite from true to false is the single most breaking change in this stack. engine.SetValue("obj", dto); engine.Execute("obj.Name = 'x'") is the bread-and-butter embedding pattern, and every host doing it must now add AllowClrWrite(). I agree with the direction — script mutating host state should be an explicit grant — but this cannot ship in a patch/minor release. It needs a major version, a first-line release-notes entry, and ideally the migration one-liner in the exception message itself (the TypeError currently says "Cannot assign to read only property 'X'", which reads like a script bug; something like "…direct CLR writes are disabled by default, see Options.AllowClrWrite" would route embedders straight to the fix).
Two smaller notes:
ListOperations.EnsureCapacitychanged meaning from capacity reservation to observable length growth (routed throughSetLength). UnderAllowClrWrite()the end state of the array ops is the same, but a mid-operation failure can now leave default-filled elements where it previously left capacity. Worth a comment at the call sites that rely on fill-after-grow.- Sloppy-mode assignments through the reflection descriptor now throw
TypeErrorwhere the spec would silently ignore a failed write — a deliberate, contained interop divergence in the loud direction; worth one line in the README's breaking-change section so nobody files it as a bug.
Stack merge still gated on the #3035 fix below.
48d371d to
9b4fcd4
Compare
9b4fcd4 to
b9d9142
Compare
…lly has Main has moved 181 commits past v4.16.0 and nothing records what an embedder has to react to. Most of it is invisible to a compiler: every entry below still compiles exactly as it did in 4.16 and behaves differently at run time, and six of them flip a default. `docs/v5-migration.md` is the artefact those entries go in. It is deliberately not a second README: a table per change, before/after code where it helps, and the rationale left in the pull request it cites. Seeded from git history, every claim checked against the tree rather than against the pull request title: * sebastienros#3054 `Interop.AllowWrite` true -> false. A projected CLR write is now silently ignored in sloppy mode and a TypeError in strict mode. * sebastienros#3056 `Interop.ArrayConversion` LiveView -> Copy. `Array.isArray` flips to true, `push`/`length` stop throwing, and CLR-side mutations after the crossing stop being visible. * sebastienros#3057 `Constraints.StackOverflowGuard` false -> true. RangeError instead of a process that ends with no exception in the log. * sebastienros#3058 `AgentCanSuspend` true -> false. `Atomics.wait` is a TypeError before a waiter is registered; `waitAsync` is untouched. * sebastienros#3052 namespace type discovery loses its implicit fallback to `Assembly.GetCallingAssembly()` / `GetExecutingAssembly()` / `Type.GetType(name)` and becomes the `AllowedAssemblies` allow-list. * sebastienros#3051 host exception, module-load and CLR-resolution messages are redacted from script; `ExposeDetailedErrors()` restores all three. * sebastienros#3035 concurrent `Engine` use fails fast, and an engine stays reserved for the lifetime of a returned async Task. * sebastienros#3036 `LimitMemory` charges allocations across async continuations, so a budget can now trip where one synchronous segment never reached it. * sebastienros#3252 every `*Async` entry reports the operation's failures through the task and only a usage error out of the call. * sebastienros#3248 an array-like `length` above 2^32-1 stops answering differently per target framework. * sebastienros#3037, sebastienros#3045, sebastienros#3046 add parser, module-graph and result bounds that all default to unlimited, so they change nothing until configured; sebastienros#3059 and sebastienros#3060 add the diagnostics and the hardened profile on top. Two entries the prompt for this work had slightly differently, both checked and written as the tree has them: the stack-overflow guard raises a catchable `RangeError`, not a `RecursionDepthOverflowException`, and `ArrayOperations` is internal, so sebastienros#3248 removes no public member — its break is what a script sees. Sections 2, 3 and 6 (removed API, renamed API, AOT) are explicit empty placeholders. Nothing public has been removed or renamed since v4.16.0, and a parallel task is measuring the AOT state. The target-framework section is written and marked pending: `Jint.csproj` still lists net462, and the net472 change is not on main yet. `Jint/AGENTS.md` gains the rule that makes the guide stay current — a change to anything in its public-contract table is a row in the guide, in the same pull request, including a change that breaks nothing at compile time. The root `AGENTS.md` is untouched; it is at 23 KB against a 24 KiB budget. README's "Branches and releases" described `main` alone and did not mention the `3.x` branch at all. It now has a row per live branch, in the same wording sebastienros#3291 gives the 4.x branch's own copy, plus a pointer to the guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S
…lly has (#3294) Main has moved 181 commits past v4.16.0 and nothing records what an embedder has to react to. Most of it is invisible to a compiler: every entry below still compiles exactly as it did in 4.16 and behaves differently at run time, and six of them flip a default. `docs/v5-migration.md` is the artefact those entries go in. It is deliberately not a second README: a table per change, before/after code where it helps, and the rationale left in the pull request it cites. Seeded from git history, every claim checked against the tree rather than against the pull request title: * #3054 `Interop.AllowWrite` true -> false. A projected CLR write is now silently ignored in sloppy mode and a TypeError in strict mode. * #3056 `Interop.ArrayConversion` LiveView -> Copy. `Array.isArray` flips to true, `push`/`length` stop throwing, and CLR-side mutations after the crossing stop being visible. * #3057 `Constraints.StackOverflowGuard` false -> true. RangeError instead of a process that ends with no exception in the log. * #3058 `AgentCanSuspend` true -> false. `Atomics.wait` is a TypeError before a waiter is registered; `waitAsync` is untouched. * #3052 namespace type discovery loses its implicit fallback to `Assembly.GetCallingAssembly()` / `GetExecutingAssembly()` / `Type.GetType(name)` and becomes the `AllowedAssemblies` allow-list. * #3051 host exception, module-load and CLR-resolution messages are redacted from script; `ExposeDetailedErrors()` restores all three. * #3035 concurrent `Engine` use fails fast, and an engine stays reserved for the lifetime of a returned async Task. * #3036 `LimitMemory` charges allocations across async continuations, so a budget can now trip where one synchronous segment never reached it. * #3252 every `*Async` entry reports the operation's failures through the task and only a usage error out of the call. * #3248 an array-like `length` above 2^32-1 stops answering differently per target framework. * #3037, #3045, #3046 add parser, module-graph and result bounds that all default to unlimited, so they change nothing until configured; #3059 and #3060 add the diagnostics and the hardened profile on top. Two entries the prompt for this work had slightly differently, both checked and written as the tree has them: the stack-overflow guard raises a catchable `RangeError`, not a `RecursionDepthOverflowException`, and `ArrayOperations` is internal, so #3248 removes no public member — its break is what a script sees. Sections 2, 3 and 6 (removed API, renamed API, AOT) are explicit empty placeholders. Nothing public has been removed or renamed since v4.16.0, and a parallel task is measuring the AOT state. The target-framework section is written and marked pending: `Jint.csproj` still lists net462, and the net472 change is not on main yet. `Jint/AGENTS.md` gains the rule that makes the guide stay current — a change to anything in its public-contract table is a row in the guide, in the same pull request, including a change that breaks nothing at compile time. The root `AGENTS.md` is untouched; it is at 23 KB against a 24 KiB budget. README's "Branches and releases" described `main` alone and did not mention the `3.x` branch at all. It now has a row per live branch, in the same wording #3291 gives the 4.x branch's own copy, plus a pointer to the guide. Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… error, not the CLR's own (backport of #3385) (#3556) A wrapped collection that declares itself read-only raised NotSupportedException out of Evaluate when script asked it to change - uncatchable by a script try/catch and by a host catch (JavaScriptException) alike. Six shapes leaked: ReadOnlyCollection<T> and a host IList<T> whose IsReadOnly is true through GenericListWrapper<T>, ImmutableList<T>, ImmutableArray<T> and anything reaching the engine as IReadOnlyList<T> through ReadOnlyListWrapper<T> (whose mutators were literally Throw.NotSupportedException), and ArrayList.ReadOnly through the untyped ListWrapper. pop and splice mutated the target part-way before the CLR refused. The refusal is now the engine's, and it is not the same refusal for every operation. push, pop, splice, sort and reverse are specified as Set(O, k, v, true) and DeletePropertyOrThrow, so they raise a TypeError in either mode; a bare assignment - host[0] = 9, host.length = 5, delete host[0] - is an ordinary [[Set]] or [[Delete]] returning false, which is a TypeError in strict mode and silent in sloppy mode. Making everything throw would have been wrong in exactly those five rows, and the collection is left untouched in all of them. ArrayLikeWrapper gains IsReadOnly, CanWrite becomes AllowWrite && !IsReadOnly and stops being virtual, and the three lanes that consulted Options.Interop.AllowWrite directly - Delete, SetAt, and ArrayOperations' array-like Set - consult CanWrite instead. Those three are exactly what made the mutators reachable for a read-only target, so the mutators are now unreachable and their bodies are a documented backstop rather than the fix. ICollection<T>.IsReadOnly cannot be believed on its face. System.Array and ArraySegment<T> both report true through it to mean "cannot grow" - the non-generic IList asks the two questions separately and the generic one collapsed them - and both accept element writes. They are therefore treated as fixed-size, which reclassifies ArraySegment<T>: it leaked NotSupportedException from push, pop, splice and a length write, and ArgumentOutOfRangeException from a write past the end, and now answers exactly as an int[] live view does while keeping arr[0] = 9. Two adaptations this branch needs that main did not. ThrowFixedSize moves from ArrayWrapper<T> up to ArrayLikeWrapper, unchanged, because the shared read-only/fixed-size guard needs it; and the integral-index refusal in Set drops its numValue < Length bound, which on main came from #3054's write-disable work. Without that bound removed a read-only target's growth write - which is what push is - still fell through to the reflected indexer and leaked the collection's own exception for the two shapes whose indexer setter throws rather than being get-only, a host IList<T> and ArrayList.ReadOnly. It changes nothing else on this branch: ObjectWrapper.Set already refuses when AllowWrite is off or the wrapper is not extensible. Jint.Tests.PublicInterface/HostReadOnlyCollectionTests.cs pins the matrix from the embedder's side: 52 of its 68 cases failed on unmodified 4.x - every one of them a System.NotSupportedException escaping Engine.Evaluate - on both net10.0 and net472, and all 68 pass now. Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tion's elements (#3561) Fixes #3558. `Options.Interop.TypeResolver.MemberFilter` is CLR-containment configuration — it is how a host says which members script may reach — and a member it rejects reads as `undefined` and cannot be written. That held for the indexer of an ordinary wrapped object, which resolves a member per access. It did not hold for a wrapped **collection**. `ArrayLikeWrapper` answers every index-shaped key itself, which is the whole point of #3416 and #3384: without it an out-of-range `list[3] = 9` was the collection's own `ArgumentOutOfRangeException` out of `Evaluate`. But that view was never told what the filter had decided, so the same filter, asked the same question, gave two answers depending on whether Jint happened to build a view. Three refusals had become writes since #3416 (`list[0] = 42`, `list['0'] = 42`, and growth `list[3] = 42`), and reads, `delete`, `push` and `sort` had bypassed the filter for longer than that. The existing `HostIndexerFilterTests.AMemberFilterExcludingTheIndexerBlocksIndexedWrites` asserted the contract and passed on `main` only because its engine left `Options.Interop.AllowWrite` at the `false` #3054 made it default to; with writes on, it fails. The whole element contract is closed rather than only the write half. Under a filter that hides the indexer, an array-like view now has no element properties at all: `Get` reads `undefined`, `in` is `false` (agreeing with `hasOwnProperty` and `Object.keys`, which already said so — `OrdinaryHasProperty` is defined in terms of `[[GetOwnProperty]]`, so they may not disagree), `Set` and `DefineOwnProperty` refuse, `delete` returns `true` without touching the slot, a `length` write neither grows nor truncates, and `ArrayOperations.For` routes every `Array.prototype` generic to `ObjectOperations` exactly as a countable-but-not-indexable `Queue<T>` is routed. Containment is asked **before** the read-only and fixed-size refusals of #3382/#3385 so the two compose rather than mask each other: a fixed-size array whose indexer is hidden reports "no such property" rather than the `TypeError` naming its bounds, which would answer a question the host never granted. Three lanes are deliberately left out of the contract, and say so. `length` is produced from `Count`, a member the filter decides about separately. Iteration is `GetEnumerator`'s business, so `[...list]` still yields elements — the shape a `Queue<T>` has always had. And `ArrayConversionMode.Copy`, the default, turns a `T[]` into a JavaScript array before any member is accessed; that is a conversion of a value, not an access to a member. The decision is the one `IndexerAccessor.TryFindIndexer` would have made — the first integer-keyed indexer the exposed type declares, falling back to the descriptor's `IList.Item` for a `T[]`, which declares none of its own — memoized per resolver and per type behind `TypeResolver.ExposesIndexedElements` and dropped with the rest of the resolved state when the filter is reassigned. A resolver with the default filter returns from one bool field read and caches nothing; a filtered one pays one dictionary lookup per array-like wrapper construction, beside the `TypeDescriptor.Get` the base constructor already does, and every element access afterwards reads a `readonly bool` field. No per-operation allocation and no per-operation delegate invocation anywhere. `docs/v5-migration.md` §4.97 carries the embedder-facing form, including what an allow-list filter has to add to keep the elements it was reaching by accident. Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… filter that hides the indexer hides it Backport of eight main pull requests that together decide one question — what an index-shaped key means on a wrapped CLR collection — plus the fix for the containment hole the seventh of them opened. They are one unit: each of the first seven moves the answer, and taking any of them alone leaves the lanes disagreeing with each other. sebastienros#3356 a host collection with a count is not a host collection with an index sebastienros#3381 a degraded array view still refuses a resize sebastienros#3425 the IndexWrappedOperations lane is not AOT-only, and a generic that must grow refuses sebastienros#3416 an index on a host collection is one property, however script spelled it sebastienros#3464 hasOwnProperty and "in" give one answer about an index on a wrapped collection sebastienros#3472 an index outside a wrapped collection is refused, not handed to the collection sebastienros#3480 a collection exposed as IList<T> or IReadOnlyList<T> gets the wrapper that contract names sebastienros#3561 a member filter that hides an indexer hides a wrapped collection's elements (fixes sebastienros#3558) sebastienros#3385 - a read-only host collection refuses script with a JavaScript error - is the ninth member of the cluster and is already on this branch as sebastienros#3556, so its hunks are not here. Its suite, HostReadOnlyCollectionTests, is 68 of 68 green both before and after this change, which is what says so. What script sees. An index-shaped key is now the view's own property, whichever way it is spelled and whether or not the position exists. A read outside the range is undefined rather than the collection's own ArgumentOutOfRangeException out of Evaluate; a write at the end grows a growable target exactly as a "length" write of the same size does; "in", hasOwnProperty, propertyIsEnumerable and getOwnPropertyDescriptor give one answer, because OrdinaryHasProperty is defined in terms of [[GetOwnProperty]] and may not disagree with it; a delete of an absent position succeeds without reaching the collection; and a countable-but-not-indexable target - Queue<T>, Stack<T>, LinkedList<T>, SortedSet<T> - is array-like with no element at index 0 rather than an InvalidCastException from a lane that cast it to IList. The containment half is why sebastienros#3561 is in the same change. Options.Interop.TypeResolver.MemberFilter is how a host says which members script may reach, and an ArrayLikeWrapper answers every index-shaped key itself, so the filter's decision about the indexer never reached the element lanes. On this branch that matters more than it does on main: Interop.AllowWrite ships on here, so a filter that hid the indexer stopped nothing. Measured on this branch, with the cluster applied and sebastienros#3561 held back, three refusals had become writes (list[0] = 42, list['0'] = 42 and growth list[3] = 42) and reads, "in", delete, push and sort had never been covered at all - and the pre-existing HostIndexerFilterTests.AMemberFilterExcludingTheIndexerBlocksIndexedWrites, which passes on stock 4.x, fails. The whole element contract is closed rather than only the write half, and containment is asked before the read-only and fixed-size refusals of sebastienros#3382/sebastienros#3385 so the two compose: a fixed-size array whose indexer is hidden reports "no such property" rather than the TypeError naming its bounds, which would answer a question the host never granted. Evidence, on net10.0 and net472 alike (identical counts on both). Against stock 4.x with the suites in place: HostNonIndexedCollectionTests 33 of 45 failed, HostExposedCollectionTypeTests 21 of 33, HostCollectionIndexWriteTests 56 of 63, HostCollectionIndexAgreementTests 11 of 18, HostCollectionIndexBoundsTests 28 of 35, HostIndexerFilterTests 13 of 18, and 6 of the 7 new InteropTests.ClrArrayLiveView cases. All of them pass now. The containment tests run in both write configurations, because on this branch the default is the interesting one: the elements leak under AllowWrite = true and the reads leak under AllowWrite = false, and both are pinned. Deliberate divergences from main. sebastienros#3054 - which is what makes Interop.AllowWrite default to false there - is a v5 default change and stays out, so this branch keeps its Delete and ArrayOperations.Set guards and the two suites spell the write switch out where main could leave it to the default. Jint.AotExample's probes from sebastienros#3381/sebastienros#3425/sebastienros#3480 are not ported: this branch's AotExample is a 22-line stub with none of the AOT probe harness those hunks extend. docs/v5-migration.md and Jint/Runtime/Interop/AGENTS.md do not exist here, so their hunks are carried into the XML docs and comments beside the code instead. The suites are xUnit v3 here rather than the NUnit main moved to in sebastienros#3409. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S
… filter that hides the indexer hides it (#3562) Backport of eight main pull requests that together decide one question — what an index-shaped key means on a wrapped CLR collection — plus the fix for the containment hole the seventh of them opened. They are one unit: each of the first seven moves the answer, and taking any of them alone leaves the lanes disagreeing with each other. #3356 a host collection with a count is not a host collection with an index #3381 a degraded array view still refuses a resize #3425 the IndexWrappedOperations lane is not AOT-only, and a generic that must grow refuses #3416 an index on a host collection is one property, however script spelled it #3464 hasOwnProperty and "in" give one answer about an index on a wrapped collection #3472 an index outside a wrapped collection is refused, not handed to the collection #3480 a collection exposed as IList<T> or IReadOnlyList<T> gets the wrapper that contract names #3561 a member filter that hides an indexer hides a wrapped collection's elements (fixes #3558) #3385 - a read-only host collection refuses script with a JavaScript error - is the ninth member of the cluster and is already on this branch as #3556, so its hunks are not here. Its suite, HostReadOnlyCollectionTests, is 68 of 68 green both before and after this change, which is what says so. What script sees. An index-shaped key is now the view's own property, whichever way it is spelled and whether or not the position exists. A read outside the range is undefined rather than the collection's own ArgumentOutOfRangeException out of Evaluate; a write at the end grows a growable target exactly as a "length" write of the same size does; "in", hasOwnProperty, propertyIsEnumerable and getOwnPropertyDescriptor give one answer, because OrdinaryHasProperty is defined in terms of [[GetOwnProperty]] and may not disagree with it; a delete of an absent position succeeds without reaching the collection; and a countable-but-not-indexable target - Queue<T>, Stack<T>, LinkedList<T>, SortedSet<T> - is array-like with no element at index 0 rather than an InvalidCastException from a lane that cast it to IList. The containment half is why #3561 is in the same change. Options.Interop.TypeResolver.MemberFilter is how a host says which members script may reach, and an ArrayLikeWrapper answers every index-shaped key itself, so the filter's decision about the indexer never reached the element lanes. On this branch that matters more than it does on main: Interop.AllowWrite ships on here, so a filter that hid the indexer stopped nothing. Measured on this branch, with the cluster applied and #3561 held back, three refusals had become writes (list[0] = 42, list['0'] = 42 and growth list[3] = 42) and reads, "in", delete, push and sort had never been covered at all - and the pre-existing HostIndexerFilterTests.AMemberFilterExcludingTheIndexerBlocksIndexedWrites, which passes on stock 4.x, fails. The whole element contract is closed rather than only the write half, and containment is asked before the read-only and fixed-size refusals of #3382/#3385 so the two compose: a fixed-size array whose indexer is hidden reports "no such property" rather than the TypeError naming its bounds, which would answer a question the host never granted. Evidence, on net10.0 and net472 alike (identical counts on both). Against stock 4.x with the suites in place: HostNonIndexedCollectionTests 33 of 45 failed, HostExposedCollectionTypeTests 21 of 33, HostCollectionIndexWriteTests 56 of 63, HostCollectionIndexAgreementTests 11 of 18, HostCollectionIndexBoundsTests 28 of 35, HostIndexerFilterTests 13 of 18, and 6 of the 7 new InteropTests.ClrArrayLiveView cases. All of them pass now. The containment tests run in both write configurations, because on this branch the default is the interesting one: the elements leak under AllowWrite = true and the reads leak under AllowWrite = false, and both are pinned. Deliberate divergences from main. #3054 - which is what makes Interop.AllowWrite default to false there - is a v5 default change and stays out, so this branch keeps its Delete and ArrayOperations.Set guards and the two suites spell the write switch out where main could leave it to the default. Jint.AotExample's probes from #3381/#3425/#3480 are not ported: this branch's AotExample is a 22-line stub with none of the AOT probe harness those hunks extend. docs/v5-migration.md and Jint/Runtime/Interop/AGENTS.md do not exist here, so their hunks are carried into the XML docs and comments beside the code instead. The suites are xUnit v3 here rather than the NUnit main moved to in #3409. Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
AllowClrWrite()opt-in without making parameterlessAllowClr()grant write authoritySecurity review
Specialist review found and this branch fixes:
pop/shift/splicemutationSetandCreateDataPropertyOrThrowfast pathsJsonArraygrowth compatibility and .NET Framework base compatibilityA clean follow-up review reported no significant issues.
Validation
Jint/Jint.csprojbuild: net462, netstandard2.0, netstandard2.1, net8.0, net10.0git diff --checkValidation used the mandated NuGet proxy with the analyzer-only local workaround restored before publishing.
Stack
#3052 -> #3051 -> #3046 -> #3045 -> #3037 -> #3036 -> #3035 -> #3030
Base:
sebros/stack-clr-containment-medium