fix(expr): JSON.stringify key lists report cycles; document stack guard coverage (#2124, #2125)

An array replacer now copies each source object once per call, so a cycle stays a cycle and the native check throws TypeError, while shared objects still serialize. Documents which threads evaluate expressions, what the 256 KiB guard covers (tested on a 512 KiB thread), that it does not apply on wasm32, and why eval is indirect.

crates/expr/src/prelude.js

@@ -1096,6 +1096,9 @@ function nearestKey(t) { return thisProperty.nearestKey(t); }
     replace(obj, name, function () { stack(); return apply(f, this, arguments); });
   }
 
+  // Indirect on purpose: a wrapper can't make a direct eval, so `eval` sees globals and `var`s
+  // but not `let`/`const` locals of the expression. Keeping direct eval would mean leaving it
+  // unchecked; expressions practically never need their locals inside eval.
   var evalJs = eval;
   eval = function (x) {
     if (typeof x === 'string') {
@@ -1135,10 +1138,12 @@ function nearestKey(t) { return thisProperty.nearestKey(t); }
 
   // A replacer function sees every value before it is serialized: the per-level check. An array
   // replacer (a list of keys) becomes one that copies each object's listed keys, in list order.
-  var stringify = JSON.stringify;
+  // Each source object gets one copy per call, so a cycle in the source is a cycle of copies and
+  // the native check reports it (TypeError); an object shared without a cycle is still fine.
+  var stringify = JSON.stringify, MapJs = Map;
   replace(JSON, 'stringify', function (value, replacer, space) {
     var fn = typeof replacer === 'function' ? replacer : null;
-    var keys = null;
+    var keys = null, copies = new MapJs();
     if (Array.isArray(replacer)) {
       keys = [];
       for (var i = 0; i < replacer.length; i++) {
@@ -1152,8 +1157,12 @@ function nearestKey(t) { return thisProperty.nearestKey(t); }
       if (fn !== null) v = apply(fn, this, [key, v]);
       if (keys !== null && v !== null && typeof v === 'object' && !Array.isArray(v) &&
           !(v instanceof Number || v instanceof String || v instanceof Boolean)) {
-        var o = {};
-        for (var j = 0; j < keys.length; j++) if (keys[j] in v) o[keys[j]] = v[keys[j]];
+        var o = copies.get(v);
+        if (o === undefined) {
+          o = {};
+          for (var j = 0; j < keys.length; j++) if (keys[j] in v) o[keys[j]] = v[keys[j]];
+          copies.set(v, o);
+        }
         v = o;
       }
       return v;

crates/expr/src/runtime.rs

@@ -235,12 +235,17 @@ fn native_random(_: &JsValue, args: &[JsValue], ctx: &mut Context) -> JsResult<J
 }
 
 /// Stack a script run may use below [`run`] for natives that recurse once per level of a nested
-/// value (`join`, `JSON.stringify`, …; the prelude calls [`native_stack`] per level). Render
-/// threads have 2 MiB; the parser's own nesting limit ([`crate::rewrite::MAX_NESTING`]) needs
-/// most of the rest.
+/// value (`join`, `JSON.stringify`, …; the prelude calls [`native_stack`] per level). It is the
+/// same on every thread, so a value converts or fails alike in preview, export and commands.
+/// Expressions run on render, export and diagnostics threads (2 MiB, std and rayon's default)
+/// and on the main thread for commands and syntax checks (1 MiB on Windows). A debug build
+/// stops the nested-value tests within a fresh 512 KiB thread, which leaves the main thread
+/// 512 KiB for its callers. The parser's limit ([`crate::rewrite::MAX_NESTING`]) is the tighter
+/// one there: 30 levels need about 1 MiB in debug builds. On wasm32 the address of a local is on
+/// the shadow stack, not the engine's call stack, so this measures nothing useful there.
 const NATIVE_STACK: usize = 256 << 10;
 
-/// An address on the current stack.
+/// An address on the current stack (on wasm32, the shadow stack).
 fn stack_here() -> usize {
     let marker = 0u8;
     std::hint::black_box(&marker) as *const u8 as usize

crates/expr/src/tests/deep_nesting.rs

@@ -10,9 +10,14 @@ const RENDER_STACK: usize = 2 << 20;
 
 /// Evaluate `text` on the title's source text (any result converts) on a render-sized stack.
 fn eval_small_stack(text: &str) -> Result<Value, String> {
+    eval_on_stack(text, RENDER_STACK)
+}
+
+/// Evaluate `text` on the title's source text on a thread with `size` bytes of stack.
+fn eval_on_stack(text: &str, size: usize) -> Result<Value, String> {
     let text = text.to_string();
     std::thread::Builder::new()
-        .stack_size(RENDER_STACK)
+        .stack_size(size)
         .spawn(move || {
             let f = fx();
             f.ev(f.title, "text/sourceText", &text, 0.0)
@@ -102,3 +107,22 @@ fn nested_values_still_convert() {
     assert_eq!(f.text("String([[[[[[[[1]]]]]]]])", 0.0), "1");
     assert_eq!(f.num("[1, [2, [3, [4]]]].flat(Infinity).length", 0.0), 4.0);
 }
+
+/// The main thread runs commands and syntax checks: 1 MiB on Windows. Half of it, for the
+/// callers above the evaluation, must still turn the nested-value triggers into errors.
+#[test]
+fn nested_values_are_an_error_on_half_the_main_thread() {
+    for tail in ["a.join()", "String([a])", "JSON.stringify(a)", "a.toLocaleString()"] {
+        let text = format!("{DEEP_ARRAY} {tail}");
+        assert!(eval_on_stack(&text, 512 << 10).is_err(), "{tail}");
+    }
+}
+
+#[test]
+fn stringify_with_a_key_list_reports_cycles_and_allows_shared_objects() {
+    let f = fx();
+    let cyclic = f.text("var o = {a: 1}; o.self = o; try { JSON.stringify(o, ['a', 'self']) } catch (e) { e.name }", 0.0);
+    assert_eq!(cyclic, "TypeError");
+    let shared = f.text("var x = {a: 1, b: 2}; JSON.stringify({p: x, q: x}, ['p', 'q', 'a'])", 0.0);
+    assert_eq!(shared, r#"{"p":{"a":1},"q":{"a":1}}"#);
+}