address review: cap whole-reference strings and the resolved parameters of a step; range adjustment uses the inserted length

crates/engine/src/cmd/variables.rs

@@ -53,9 +53,9 @@ pub fn specs() -> Vec<CommandSpec> {
                         }
                         let st = d.story_mut(sid).ok_or(designcraft_doc::DocError::NoStory(sid))?;
                         let mut replacements = Vec::new();
-                        unvar(st, i, n, &value, 0, &mut |pos, len| replacements.push((pos, len)));
-                        for (pos, tlen) in replacements {
-                            d.adjust_text_ranges(sid, pos..pos + tlen, value.len());
+                        unvar(st, i, n, &value, 0, &mut |pos, len, ins| replacements.push((pos, len, ins)));
+                        for (pos, tlen, ins) in replacements {
+                            d.adjust_text_ranges(sid, pos..pos + tlen, ins);
                         }
                     }
                     d.text_variables.remove(i);
@@ -356,7 +356,7 @@ fn needs_unvar(st: &designcraft_doc::Story, i: usize, depth: usize) -> bool {
 /// `i + 1..n` down by one, including table cells and foot/endnotes nested in the story. Only
 /// touches (and unshares) the parts that hold such instances; a story whose nested text changed
 /// gets a new revision.
-fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth: usize, on_replace: &mut dyn FnMut(usize, usize)) {
+fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth: usize, on_replace: &mut dyn FnMut(usize, usize, usize)) {
     if depth > MAX_DEPTH {
         return;
     }
@@ -374,7 +374,7 @@ fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth
             let value = var_char(i).map_or_else(|| value.to_string(), |c| value.replace(c, ""));
             for pos in at {
                 st.replace(pos..pos + target.len(), &value);
-                on_replace(pos, target.len());
+                on_replace(pos, target.len(), value.len());
             }
         }
         for k in i + 1..n {
@@ -391,14 +391,14 @@ fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth
         }
         for c in &mut std::sync::Arc::make_mut(t).cells {
             if needs_unvar(&c.text, i, depth + 1) {
-                unvar(&mut c.text, i, n, value, depth + 1, &mut |_, _| {});
+                unvar(&mut c.text, i, n, value, depth + 1, &mut |_, _, _| {});
             }
         }
         nested = true;
     }
     for note in st.notes.iter_mut().chain(st.endnotes.iter_mut()) {
         if needs_unvar(&note.text, i, depth + 1) {
-            unvar(&mut std::sync::Arc::make_mut(note).text, i, n, value, depth + 1, &mut |_, _| {});
+            unvar(&mut std::sync::Arc::make_mut(note).text, i, n, value, depth + 1, &mut |_, _, _| {});
             nested = true;
         }
     }

crates/engine/src/cmd/variables/tests.rs

@@ -301,10 +301,17 @@ fn delete_variable_whose_value_holds_its_marker_ends() {
     let a = s.doc().unwrap().doc.variable("A").unwrap();
     let marker = var_char(a).unwrap();
     s.execute("variables.define", &json!({"name": "A", "type": "custom", "text": format!("<{marker}>")})).unwrap();
-    s.execute("frame.create", &json!({"rect": [72, 72, 400, 300], "content": "text", "text": format!("x{marker}y{marker}z")})).unwrap();
+    let r = s.execute("frame.create", &json!({"rect": [72, 72, 400, 300], "content": "text", "text": format!("x{marker}y{marker}z")})).unwrap();
+    let sid = s.doc().unwrap().doc.item(designcraft_doc::ItemId(r["id"].as_u64().unwrap())).unwrap().text_frame().unwrap().story;
+    // A link on the last "z": its range follows the text put in (without the marker).
+    let z = format!("x{marker}y{marker}").len();
+    s.execute("text.select", &json!({"story": sid.0, "anchor": z, "focus": z + 1})).unwrap();
+    s.execute("hyperlink.create", &json!({"url": "https://example.org"})).unwrap();
     s.execute("variables.delete", &json!({"name": "A"})).unwrap();
     let d = &s.doc().unwrap().doc;
     assert_eq!(d.stories.values().next().unwrap().text, "x<>y<>z");
+    let designcraft_doc::HyperlinkSource::Text { start, end, .. } = &d.hyperlinks[0].source else { panic!("a text link") };
+    assert_eq!((*start, *end), (6, 7));
 }
 
 /// Deleting a variable flattens its instances in footnotes and endnotes too and moves the

crates/engine/src/script.rs

@@ -120,14 +120,49 @@ fn too_long(s: &str) -> String {
     format!("the interpolated text of {preview} is longer than {MAX_INTERPOLATED_BYTES} bytes").replace('\n', " ")
 }
 
+/// Longest string a whole reference (`$last.text`) may copy in. A story is far smaller; the cap
+/// stops a loop of "read the story, paste it back" from doubling it until memory runs out.
+pub const MAX_REFERENCED_BYTES: usize = 64 << 20;
+
+/// Most bytes of strings one step's parameters may hold once its references are resolved: the
+/// limits above apply to each string, this one to a whole parameter tree (an array of 256
+/// references to a large string would pass each of them).
+pub const MAX_STEP_PARAM_BYTES: usize = 256 << 20;
+
+/// Roughly how many bytes cloning `v` allocates (its strings, plus a word per node).
+fn weight(v: &Value) -> usize {
+    match v {
+        Value::String(s) => s.len(),
+        Value::Array(a) => a.iter().fold(8, |n, x| n.saturating_add(weight(x))),
+        Value::Object(o) => o.iter().fold(8, |n, (k, x)| n.saturating_add(k.len()).saturating_add(weight(x))),
+        _ => 8,
+    }
+}
+
+fn charge(budget: &mut usize, bytes: usize) -> Result<(), String> {
+    *budget =
+        budget.checked_sub(bytes).ok_or_else(|| format!("the parameters of a step are longer than {MAX_STEP_PARAM_BYTES} bytes once resolved"))?;
+    Ok(())
+}
+
 /// Replace references in `params` with values from earlier `results`.
 pub fn resolve(params: &Value, results: &[Value]) -> Result<Value, String> {
+    resolve_in(params, results, MAX_REFERENCED_BYTES, &mut { MAX_STEP_PARAM_BYTES })
+}
+
+fn resolve_in(params: &Value, results: &[Value], max_ref: usize, budget: &mut usize) -> Result<Value, String> {
     Ok(match params {
         Value::String(s) => {
             if let Some(r) = s.strip_prefix('$').filter(|r| is_reference(r)) {
-                return lookup(r, results).cloned();
+                let v = lookup(r, results)?;
+                if matches!(v, Value::String(t) if t.len() > max_ref) {
+                    return Err(format!("`${r}` is longer than {max_ref} bytes"));
+                }
+                charge(budget, weight(v))?;
+                return Ok(v.clone());
             }
             if !s.contains("${") && !s.contains("$$") {
+                charge(budget, s.len())?;
                 return Ok(params.clone());
             }
             let mut out = String::new();
@@ -159,10 +194,13 @@ pub fn resolve(params: &Value, results: &[Value]) -> Result<Value, String> {
                 return Err(too_long(s));
             }
             out.push_str(rest);
+            charge(budget, out.len())?;
             Value::String(out)
         }
-        Value::Array(a) => Value::Array(a.iter().map(|v| resolve(v, results)).collect::<Result<_, _>>()?),
-        Value::Object(o) => Value::Object(o.iter().map(|(k, v)| Ok((k.clone(), resolve(v, results)?))).collect::<Result<_, String>>()?),
+        Value::Array(a) => Value::Array(a.iter().map(|v| resolve_in(v, results, max_ref, budget)).collect::<Result<_, _>>()?),
+        Value::Object(o) => {
+            Value::Object(o.iter().map(|(k, v)| Ok((k.clone(), resolve_in(v, results, max_ref, budget)?))).collect::<Result<_, String>>()?)
+        }
         v => v.clone(),
     })
 }
@@ -240,6 +278,18 @@ mod tests {
 
     use super::*;
 
+    /// A whole reference to a long string is refused, and the strings of one step's parameters
+    /// share a budget (small limits here: the real ones would mean allocating them).
+    #[test]
+    fn whole_references_and_the_parameter_tree_are_bounded() {
+        let results = vec![json!({"text": "x".repeat(100)})];
+        assert!(resolve_in(&json!("$0.text"), &results, 100, &mut 1000).is_ok());
+        assert!(resolve_in(&json!("$0.text"), &results, 99, &mut 1000).unwrap_err().contains("longer than 99"));
+        let many = json!(["$0.text", "$0.text", "${0.text}"]);
+        assert!(resolve_in(&many, &results, 100, &mut 300).is_ok());
+        assert!(resolve_in(&many, &results, 100, &mut 299).unwrap_err().contains("once resolved"));
+    }
+
     #[test]
     fn leading_blank_lines_keep_line_numbers() {
         let e = parse("\n\n# c\nfile.new\nframe.create {bad").unwrap_err();