From 52e110b67ca891f3720232b3ca863ad941961491 Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Sun, 26 Jul 2026 12:41:12 +0200 Subject: [PATCH 1/4] fix(dom): guard DomTree::remove() against OOB and double-free remove() freed a node and its descendants by directly indexing inner.nodes and unconditionally pushing each slot onto the free list. An out-of-range id panicked out-of-bounds, and removing an already-freed node pushed its slot a second time, so two later allocations could be handed the same NodeId (aliasing/corruption). Free a slot only when it is currently live, via checked access. Fixes #500 --- crates/obscura-dom/src/tree.rs | 40 ++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index 01db8af..8ca2475 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -469,12 +469,20 @@ impl DomTree { inner.id_index.remove(&id_str); } + // Only free slots that are currently live. Freeing an out-of-range id + // would panic on direct indexing, and freeing an already-freed slot + // would push it onto the free list a second time — later handing the + // same NodeId to two live nodes (aliasing). for desc_id in descendants { - inner.nodes[desc_id.index()] = None; - inner.free_list.push(desc_id.0); + if matches!(inner.nodes.get(desc_id.index()), Some(Some(_))) { + inner.nodes[desc_id.index()] = None; + inner.free_list.push(desc_id.0); + } + } + if matches!(inner.nodes.get(node_id.index()), Some(Some(_))) { + inner.nodes[node_id.index()] = None; + inner.free_list.push(node_id.0); } - inner.nodes[node_id.index()] = None; - inner.free_list.push(node_id.0); } pub fn children(&self, node_id: NodeId) -> Vec { @@ -932,6 +940,30 @@ mod tests { assert!(node.is_document()); } + #[test] + fn remove_out_of_range_id_is_a_noop() { + let tree = DomTree::new(); + // Direct indexing into `nodes` panicked out-of-bounds for an id past the + // end of the slot vector; it must be a no-op instead. + tree.remove(NodeId::new(9999)); + assert_eq!(tree.len(), 1); + } + + #[test] + fn remove_twice_does_not_alias_slots() { + let tree = DomTree::new(); + let doc = tree.document(); + let a = tree.new_node(NodeData::Text { contents: "a".into() }); + tree.append_child(doc, a); + tree.remove(a); + // Removing the already-freed node again must not push its slot onto the + // free list a second time, or two later allocations alias one slot. + tree.remove(a); + let x = tree.new_node(NodeData::Text { contents: "x".into() }); + let y = tree.new_node(NodeData::Text { contents: "y".into() }); + assert_ne!(x, y, "double-free aliased two live nodes onto the same slot"); + } + #[test] fn test_append_child() { let tree = DomTree::new(); From 72a9c3ced3f09d39a03d38a3ff7aba6a6d0e9ae4 Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Sun, 26 Jul 2026 12:46:03 +0200 Subject: [PATCH 2/4] fix(dom): honor quirks mode for class/id selector case-sensitivity set_quirks_mode was a no-op and the selector MatchingContext hardcoded QuirksMode::NoQuirks, so class/id selectors were always case-sensitive even on quirks-mode (no-doctype) pages where browsers match .foo / #bar ASCII-case-insensitively. Store the parser-reported quirks mode on DomTree, pass it into the selector context, and skip the exact-case #id index fast path in quirks mode so it falls through to case-insensitive matching. Fixes #499 --- crates/obscura-dom/src/selector.rs | 67 +++++++++++++++++++++++------ crates/obscura-dom/src/tree.rs | 15 +++++++ crates/obscura-dom/src/tree_sink.rs | 5 ++- 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/crates/obscura-dom/src/selector.rs b/crates/obscura-dom/src/selector.rs index 4c5acf4..6f85797 100644 --- a/crates/obscura-dom/src/selector.rs +++ b/crates/obscura-dom/src/selector.rs @@ -580,16 +580,21 @@ impl DomTree { // Fast path: a bare "#id" selector resolves through the id index in O(1) // instead of scanning every descendant. The index holds the first element // in tree order per id, which is exactly what the full scan would return. - if let Some(id) = simple_id_selector(selector) { - match self.get_element_by_id(id) { - // querySelector matches strict descendants of root only, so the - // indexed element must have root among its ancestors. - Some(nid) if self.ancestors(nid).contains(&root) => return Ok(Some(nid)), - // Index miss or stale entry (detached node) — fall through to full scan. - // The id_index is best-effort: it only registers nodes at creation time - // and does not update on reparent, so it can point to a detached clone - // while the live node (with the same id) is elsewhere in the tree. - _ => {} + // In quirks mode `#id` matches ASCII-case-insensitively, but the id + // index is keyed on the exact-case id, so skip the fast path and let the + // selector engine (below) do the case-insensitive match. + if !self.is_quirks() { + if let Some(id) = simple_id_selector(selector) { + match self.get_element_by_id(id) { + // querySelector matches strict descendants of root only, so the + // indexed element must have root among its ancestors. + Some(nid) if self.ancestors(nid).contains(&root) => return Ok(Some(nid)), + // Index miss or stale entry (detached node) — fall through to full scan. + // The id_index is best-effort: it only registers nodes at creation time + // and does not update on reparent, so it can point to a detached clone + // while the live node (with the same id) is elsewhere in the tree. + _ => {} + } } } let selector_list = parse_selector(selector)?; @@ -598,7 +603,7 @@ impl DomTree { MatchingMode::Normal, None, &mut caches, - QuirksMode::NoQuirks, + self.selector_quirks_mode(), NeedsSelectorFlags::No, MatchingForInvalidation::No, ); @@ -619,6 +624,16 @@ impl DomTree { Ok(None) } + // Map the document's quirks flag onto the selector crate's QuirksMode. In + // quirks mode the crate matches class/id selectors ASCII-case-insensitively. + fn selector_quirks_mode(&self) -> QuirksMode { + if self.is_quirks() { + QuirksMode::Quirks + } else { + QuirksMode::NoQuirks + } + } + pub fn query_selector_all_from(&self, root: NodeId, selector: &str) -> Result, String> { let selector_list = parse_selector(selector)?; let mut caches = selectors::context::SelectorCaches::default(); @@ -626,7 +641,7 @@ impl DomTree { MatchingMode::Normal, None, &mut caches, - QuirksMode::NoQuirks, + self.selector_quirks_mode(), NeedsSelectorFlags::No, MatchingForInvalidation::No, ); @@ -729,6 +744,34 @@ mod tests { assert!(result.is_none()); } + #[test] + fn quirks_mode_matches_class_and_id_case_insensitively() { + // No doctype => quirks mode; class/id match ASCII case-insensitively. + let tree = parse_html(r#"
x
"#); + assert!( + tree.query_selector(".foo").unwrap().is_some(), + ".foo should match class=\"Foo\" in quirks mode" + ); + assert!( + tree.query_selector("#bar").unwrap().is_some(), + "#bar should match id=\"Bar\" in quirks mode" + ); + } + + #[test] + fn standards_mode_matches_class_and_id_case_sensitively() { + // With a doctype => no-quirks; class/id remain case-sensitive. + let tree = parse_html(r#"
x
"#); + assert!( + tree.query_selector(".foo").unwrap().is_none(), + ".foo must NOT match class=\"Foo\" in standards mode" + ); + assert!( + tree.query_selector("#bar").unwrap().is_none(), + "#bar must NOT match id=\"Bar\" in standards mode" + ); + } + #[test] fn test_query_selector_complex() { let tree = parse_html( diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index 01db8af..2667206 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -143,6 +143,9 @@ pub(crate) struct DomTreeInner { pub(crate) free_list: Vec, pub(crate) document: NodeId, pub(crate) id_index: HashMap, + // Whether the document was parsed in (full) quirks mode. In quirks mode CSS + // class and id selectors match ASCII-case-insensitively. + pub(crate) quirks: bool, } impl DomTree { @@ -162,6 +165,7 @@ impl DomTree { free_list: Vec::new(), document: NodeId(0), id_index: HashMap::new(), + quirks: false, }), } } @@ -170,6 +174,17 @@ impl DomTree { self.inner.borrow().document } + /// Record whether the document was parsed in (full) quirks mode. + pub fn set_quirks(&self, quirks: bool) { + self.inner.borrow_mut().quirks = quirks; + } + + /// Whether the document is in (full) quirks mode, in which CSS class and id + /// selectors match ASCII-case-insensitively. + pub fn is_quirks(&self) -> bool { + self.inner.borrow().quirks + } + pub(crate) fn borrow_inner(&self) -> std::cell::Ref<'_, DomTreeInner> { self.inner.borrow() } diff --git a/crates/obscura-dom/src/tree_sink.rs b/crates/obscura-dom/src/tree_sink.rs index 674732f..e116b5a 100644 --- a/crates/obscura-dom/src/tree_sink.rs +++ b/crates/obscura-dom/src/tree_sink.rs @@ -218,7 +218,10 @@ impl TreeSink for DomTree { x == y } - fn set_quirks_mode(&self, _mode: QuirksMode) { + fn set_quirks_mode(&self, mode: QuirksMode) { + // Only full quirks mode makes CSS class/id selectors case-insensitive; + // limited-quirks behaves like no-quirks for selector matching. + self.set_quirks(mode == QuirksMode::Quirks); } fn is_mathml_annotation_xml_integration_point(&self, target: &NodeId) -> bool { From 499a5ef983d308d3cf9b95830b3013cdead1b496 Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Sun, 26 Jul 2026 12:33:25 +0200 Subject: [PATCH 3/4] fix(js): coerce non-string primitives in before/after/replaceWith before(), after() and replaceWith() only converted string arguments to Text nodes; other primitives (numbers, booleans, null) were passed straight to insert_before with no _nid, corrupting the call. Route them through the shared _convertNodes helper (the same path append/prepend use), so every non-node argument becomes document.createTextNode(String(n)). Fixes #498 --- crates/obscura-js/js/bootstrap.js | 18 ++++++------------ crates/obscura-js/src/runtime.rs | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/crates/obscura-js/js/bootstrap.js b/crates/obscura-js/js/bootstrap.js index 3ef371a..96353c0 100644 --- a/crates/obscura-js/js/bootstrap.js +++ b/crates/obscura-js/js/bootstrap.js @@ -4122,13 +4122,13 @@ if (typeof Response === 'undefined') { } if (!Element.prototype.replaceWith) { + // _convertNodes turns any non-node argument (numbers, booleans, null, …) into + // a Text node via String(n), matching the spec and append()/prepend(); the + // old `typeof n === 'string'` check corrupted insert_before for other types. Element.prototype.replaceWith = function(...nodes) { const parent = this.parentNode; if (!parent) return; - for (const n of nodes) { - if (typeof n === 'string') parent.insertBefore(document.createTextNode(n), this); - else parent.insertBefore(n, this); - } + for (const n of _convertNodes(nodes)) parent.insertBefore(n, this); parent.removeChild(this); }; _markNative(Element.prototype.replaceWith); @@ -4137,10 +4137,7 @@ if (!Element.prototype.before) { Element.prototype.before = function(...nodes) { const parent = this.parentNode; if (!parent) return; - for (const n of nodes) { - if (typeof n === 'string') parent.insertBefore(document.createTextNode(n), this); - else parent.insertBefore(n, this); - } + for (const n of _convertNodes(nodes)) parent.insertBefore(n, this); }; _markNative(Element.prototype.before); } @@ -4149,10 +4146,7 @@ if (!Element.prototype.after) { const parent = this.parentNode; if (!parent) return; const ref = this.nextSibling; - for (const n of nodes) { - if (typeof n === 'string') parent.insertBefore(document.createTextNode(n), ref); - else parent.insertBefore(n, ref); - } + for (const n of _convertNodes(nodes)) parent.insertBefore(n, ref); }; _markNative(Element.prototype.after); } diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index fdfeb4d..a6bd24c 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -1317,6 +1317,26 @@ mod tests { rt } + #[test] + fn childnode_helpers_coerce_non_string_primitives_to_text() { + let mut rt = setup_runtime(r#"
x
"#); + let before = rt + .evaluate("(function(){var t=document.getElementById('t'); t.before(5); return t.previousSibling ? t.previousSibling.textContent : 'NULL';})()") + .unwrap(); + assert_eq!(before, serde_json::json!("5")); + let after = rt + .evaluate("(function(){var t=document.getElementById('t'); t.after(true); return t.nextSibling ? t.nextSibling.textContent : 'NULL';})()") + .unwrap(); + assert_eq!(after, serde_json::json!("true")); + let replaced = rt + .evaluate("(function(){var t=document.getElementById('t'); t.replaceWith(42); return document.getElementById('p').textContent;})()") + .unwrap(); + assert!( + replaced.as_str().unwrap().contains("42"), + "replaceWith(42) should leave text '42': {replaced}" + ); + } + #[test] fn test_document_title() { let mut rt = setup_runtime("Test"); From 1fc5a24e9887d5cd83303c24f32c9fb3d01f2338 Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Sun, 26 Jul 2026 12:32:27 +0200 Subject: [PATCH 4/4] fix(js): keep the current URL when pushState/replaceState omit url resolveOrFallback(undefined) returned undefined, so applyVirtual set __virtualUrl to null and location reverted to the document URL. Per the HTML spec a missing url keeps the current document URL; return __currentUrl() so the new history entry captures the current location instead of clearing it. Fixes #496 --- crates/obscura-js/js/bootstrap.js | 5 ++++- crates/obscura-js/src/runtime.rs | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/crates/obscura-js/js/bootstrap.js b/crates/obscura-js/js/bootstrap.js index 3ef371a..e3073d8 100644 --- a/crates/obscura-js/js/bootstrap.js +++ b/crates/obscura-js/js/bootstrap.js @@ -5613,7 +5613,10 @@ globalThis.atob = globalThis.atob || ((s) => { const c="ABCDEFGHIJKLMNOPQRSTUVWX const stack = [{state: null, url: undefined}]; // initial entry; url=undefined means "use document URL" let idx = 0; const resolveOrFallback = (url) => { - if (url === null || url === undefined) return undefined; + // A missing url (pushState/replaceState called with < 3 args) keeps the + // current document URL per the HTML spec — capture it so the entry does not + // reset location back to the original document URL. + if (url === null || url === undefined) return __currentUrl(); try { return new URL(String(url), __currentUrl()).href; } catch (e) { return String(url); } }; const applyVirtual = () => { diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index fdfeb4d..5b4d27e 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -1317,6 +1317,28 @@ mod tests { rt } + #[test] + fn replace_state_without_url_preserves_current_location() { + let mut rt = setup_runtime(""); + let path = rt + .evaluate( + "(function(){history.pushState({}, '', '/dashboard'); history.replaceState({scroll:1}); return location.pathname;})()", + ) + .unwrap(); + assert_eq!(path, serde_json::json!("/dashboard")); + } + + #[test] + fn push_state_without_url_preserves_current_location() { + let mut rt = setup_runtime(""); + let path = rt + .evaluate( + "(function(){history.pushState({}, '', '/a'); history.pushState({b:1}); return location.pathname;})()", + ) + .unwrap(); + assert_eq!(path, serde_json::json!("/a")); + } + #[test] fn test_document_title() { let mut rt = setup_runtime("Test");