Repository navigation
Add module graph security limits and destination policy - #3045
Conversation
ad3c90a to
1ce9064
Compare
lahma
left a comment
There was a problem hiding this comment.
Reviewed as the incremental diff against sebros/parser-source-ast-limits. Everything here is opt-in (all four limits default to unlimited, LoadPolicy defaults to null), the single-choke-point discipline is right (GuardedResolve is the only import-driven resolution path, so the policy cannot be bypassed; registration indexing applies policy but deliberately doesn't consume hops), and the accounting is transactional — RegisterModuleWithAccounting reserves before the debugger callback can re-enter and rolls back on failure, and the registration-index pass now restores its version and in-flight marker when a policy throws mid-pass. The depth design is sensible: a real-path check during the walk for early abort, then the SCC/condensation longest-path validation for a deterministic answer independent of sibling and async completion order — I traced the Kosaraju + Kahn implementation and it holds up (root-seeded distances, the > 0 guard keeping unreachable components inert, cycles contributing their full member count). Converting InnerModuleLoading to a worklist also quietly removes native-stack recursion for deep synchronous graphs, which is a win on its own.
Two details I particularly like: the EngineAbortRegistry CWT marker that lets the pipeline distinguish the engine's own TimeoutException/OperationCanceledException from a loader's transport timeout without inventing new exception types, and ModuleLoadCompletion capturing the cancellation token at registration so a background settle never has to enter the engine to rediscover it.
The 50+ HostModuleGraphSecurityTests cover exact boundaries, dedup/diamond/cycle counting, per-operation hop budgets on pooled engines, async pump propagation, the ordinary-vs-engine cancellation split from both loader kinds, and every allowlist dimension including the separator-boundary file-root case. Approving.
Non-blocking notes:
MaxModuleCount/MaxTotalModuleSourceBytesreject non-positive values at engine construction, whereas the constraint helpers (LimitMemory,MaxStatements) treat non-positive as "remove the limit". Failing loud is arguably the better convention and the XML docs say so — just noting the deliberate divergence.ModuleAllowlistPolicy's mutableList<>properties mean a policy shared across engines can be mutated mid-load; it's engine-thread-consulted so this is the usual Options-sharing caveat, and the symlink non-guarantee is documented. Fine as is.- Depth is only authoritatively enforced when the graph finishes loading (the SCC pass), so a hostile graph inside the count/byte budgets still gets fully fetched before a depth rejection; the during-walk real-path check catches the linear-chain case early. That trade for determinism seems right — worth keeping the rationale in the XML doc as it is.
Stack merge remains gated on the #3035 fix below.
1ce9064 to
b8ce703
Compare
b8ce703 to
88ebf8e
Compare
Four security-stack layers (sebastienros#3037, sebastienros#3045, sebastienros#3046, sebastienros#3051) merged between this branch's base and its landing, and each added exactly the shape of setting the posture rule exists to classify. The parser bounds, the four module-graph limits and the ResultLimits bundle are restrictions and travel; detailed-error exposure and the require shim are grants and stay behind. Modules stops being excludable wholesale — the group now mixes its loader (a grant) with numeric limits (restrictions), so it joins the scanned set with each member classified. ResultLimits is class-typed and outside the reflective pin's value-typed scan, so it is classified in CopySecurityPosture's body instead, and the copy is by reference because the bundle is sealed and immutable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd
Four security-stack layers (sebastienros#3037, sebastienros#3045, sebastienros#3046, sebastienros#3051) merged between this branch's base and its landing, and each added exactly the shape of setting the posture rule exists to classify. The parser bounds, the four module-graph limits and the ResultLimits bundle are restrictions and travel; detailed-error exposure and the require shim are grants and stay behind. Modules stops being excludable wholesale — the group now mixes its loader (a grant) with numeric limits (restrictions), so it joins the scanned set with each member classified. ResultLimits is class-typed and outside the reflective pin's value-typed scan, so it is classified in CopySecurityPosture's body instead, and the copy is by reference because the bundle is sealed and immutable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd
#3219) * Workers: the host-facing types, the options group and the posture rule The foundation of the Worker feature (#3167), with no script surface at all: `typeof Worker` stays `undefined` on every engine, nothing is installed, and no engine runtime behaviour changes. What lands: - `Jint/WebApi/Workers/` — `WorkerProvider` (the host's answer to `new Worker(...)`, since Jint never starts a thread), `WorkerRequest` (what is being asked for, plus `CreateDefaultOptions()`), `WorkerConnection` (one live parent-worker pair) and the two enums. The connection is thread-safe now rather than later: `End()` is idempotent under *concurrent* callers, every property reads safely from any thread, and the end callback runs outside the lock so host code never runs under one. - `Options.WebApi.Workers` — `Provider` (null by default, which is what keeps the global uninstalled), `MaxWorkers` (16, a per-engine backstop and not the policy) and `MaxQueuedMessages` (16384, a backstop against a never-pumped worker rather than flow control), plus `WebApiFeatures.Workers = 1 << 24`, never in `Default`, and `UseWorkers` which sets flag and provider together. - `Options.CopySecurityPosture` — restrictions travel, grants never travel by implication. It copies the seven `Constraints` value settings, `Host.StringCompilationAllowed`, `AgentCanSuspend` and `Json.MaxParseDepth`, and lives beside the properties it names with three explicit classification lists: what is inherited, what is deliberately not (with the reason), and which option groups are excluded wholesale as grant-shaped. `CreateDefaultOptions()` returns a fresh `Options` each call: the termination token as a cancellation constraint, every parent constraint *factory* replayed (never an instance — those are single-engine-only), the copied posture, the feature mask minus network/storage/routing/nesting plus `Messaging` and `GlobalEvents`, and `DiagnosticsSink.Null`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: pin the foundation, and the classification that outlives it Mechanism in Jint.Tests, third-party reachability in Jint.Tests.PublicInterface, each pin written so that removing the mechanism it names makes it fail. `OptionsSecurityPostureTests` is the one worth keeping past this feature: it reflects over the value-typed settings of `Options` and its `Constraints`, `Host` and `Json` groups and fails unless each is either copied by `CopySecurityPosture` or named as deliberately not inherited, and it does the same for the option groups themselves. It found one on its first run — `Options.Profiling`, which is now classified (host diagnostics, script cannot reach it, and its gate already defaults to refusing). `TheDefaultOptionsCopyTheParentsSecurityPosture` is a theory with one case per copied setting, so dropping a single line of the copy fails exactly the case that names it rather than one arbitrary assertion. `TypeofWorkerIsUndefinedEvenWithTheFlagAndProvider` pins that this change is not also the one that moves the engine: with the flag on, a provider registered and the web APIs enabled, a script still cannot tell any of this exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: sync the design doc, and tell the truth about a pumped worker's budget `docs/design/web-workers.md` predates the finalized plan in #3167 and disagreed with it in nine places that matter. Rewritten decision for decision: - `close()` is not `terminate()`. The spec's *close a worker* discards the worker's own queued tasks and sets the closing flag; it pointedly does not abort the running script or empty the parent-side queue. The doc had them sharing one three-step teardown, which would kill `close(); flushMetrics();` and lose `postMessage(result); close()`. - A load failure fires a plain `Event`, not an `ErrorEvent`, and ends the connection as `StartupFailed` rather than `Terminated`. - The parent-side error relay is an internal per-connection hook gated on *notHandled*, sitting beside the `DiagnosticsSink` rather than being it — the sink is deliberately unsuppressible by script and cannot carry that gate. - Nesting is OFF by default; `MaxWorkers` (16) is a per-engine backstop that bounds a branching factor, not a tree, which is what the old text claimed. - Security posture: restrictions travel, grants never travel by implication, and the classification is pinned reflectively. - `importScripts` is present and throwing, because the spec's own step 1 for a module worker prescribes the throw. - The agent-cluster claim is corrected: HTML puts a dedicated worker in its creator's cluster, so refusing `SharedArrayBuffer` is Jint's isolation policy and not a fact about agents. - "Close both ports is already the shipped rule" is withdrawn: the shipped rule is the opposite one-sided close, and the worker rule is argued on its own merits (cost, not delivery). - The generation fence is per **port**, captured at port construction, which is what lets a transferred side rejoin the receiving engine's current cycle. - The `AWorkerCannotBeSentAMessagePort` pin is deleted; it contradicted #3197. Line-number citations are replaced with file-and-member ones throughout. Most of the old numbers were already wrong, and a wrong line number reads like a fact. #3215's transferable-streams paragraph is preserved, integrated into the transfer section beside the port transfer it rides on. The same correction lands in the XML docs, where it is load-bearing rather than merely explanatory. `WorkerRequest.CreateDefaultOptions` claimed a replayed `LimitMemory` never fires on a pumped worker. Since #3036 that is false: every event-loop job runs inside an allocation segment and is checked as it completes, carrying the operation state captured at registration across continuations and thread hops, so a replayed factory genuinely bounds each job chain. The worker budget is a pair — `OperationDeadlineConstraint` for time, `MemoryLimitConstraint` for allocations. The same change cites #3036's other consequence: instance copying is now loudly wrong, because `MemoryLimitConstraint.Attach` throws when one instance meets a second engine. `Options`'s wholesale exclusion of the `WebApi` group also stops glossing: its value settings are per-feature caps living on the sub-groups, and each rides with the feature it bounds rather than being a restriction that stayed behind by accident. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: make the End()-idempotency pin actually race The pin claimed to prove that `WorkerConnection.End()` is idempotent under *concurrent* callers rather than merely repeated ones — which is the entire reason `TryEnd` takes a lock instead of testing and setting a `volatile bool`. It did not prove it. Replacing the lock with a plain check-then-set left the test passing five times out of five: `Parallel.For` ramps its workers up, so by the time the second one starts the first has long since set the flag and there is no race left to lose. The pin named a mechanism it could not see. A `Barrier` releases every racer into the same instant instead, the threads are reused across 64 rounds so that many attempts cost a handful of threads rather than thousands, and each round gets its own connection and its own callback counter — so the failure message names the round rather than an aggregate. The unlocked version now loses within the first couple of rounds, verified five times out of five, and the assertion that fails is the callback count, which is exactly the mechanism removed. Nothing here is timed: there is no wall-clock assertion, so a slow machine makes this test slower and never redder, and the joins carry a 60-second ceiling only so that an `End()` that deadlocked fails the test instead of hanging the run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Workers: classify the security settings the stack landed under us Four security-stack layers (#3037, #3045, #3046, #3051) merged between this branch's base and its landing, and each added exactly the shape of setting the posture rule exists to classify. The parser bounds, the four module-graph limits and the ResultLimits bundle are restrictions and travel; detailed-error exposure and the require shim are grants and stay behind. Modules stops being excludable wholesale — the group now mixes its loader (a grant) with numeric limits (restrictions), so it joins the scanned set with each member classified. ResultLimits is class-typed and outside the reflective pin's value-typed scan, so it is classified in CopySecurityPosture's body instead, and the copy is by reference because the bundle is sealed and immutable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd * Workers: inherit the untrusted-code profile's expansion, never its marker #3060 landed the hardened profile while this branch was in review, and it resolved a question the design had left open in the profile's favour — almost. An untrusted parent's Engine.Options is the profile's expanded clone, so the posture copy already takes every value the expansion set and the factory replay already carries its budget constraints; propagating the marker itself would be strictly worse, because a marked options object re-expands at engine construction and that expansion clears the constraint registrations, including the cancellation constraint a worker's terminate() depends on. The unmarked worker loses only the diagnostics label, and the test pins both halves. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnghUC3EKrYa12xQ2LaYyd --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…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>
Summary
Stack
This PR targets #3037 (
sebros/parser-source-ast-limits). The security stack is:#3037 -> #3036 -> #3035 -> #3030
Validation
Jint/Jint.csprojbuild: 5/5 target frameworksJint.Tests.PublicInterface: 1643 passed, 9 skippedgit diff --checkThe approved NuGet proxy does not yet carry the target branch's newly bumped
Meziantou.Analyzer3.0.156 (nearest 3.0.141), so local post-rebase validation temporarily used the immediately preceding cached dependency versions without committing that override. The exact module source had already built and passed before the target's dependency-only update.