From 1a5c27a5924b6840ed1b621876e8d8202ff4c2ce Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Wed, 22 Jul 2026 18:57:00 +0200 Subject: [PATCH] fix(js): honour NodeFilter.FILTER_REJECT in TreeWalker (#461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_accept` collapsed every non-ACCEPT filter result into a single boolean, so FILTER_REJECT behaved exactly like FILTER_SKIP: the node was not returned, but the walk still descended into its subtree. Per DOM 6.1, FILTER_REJECT rejects a node *and* its descendants. This was unreachable until #440 exposed the real NodeFilter constants, which made the canonical prune idiom if (node.tagName === 'SCRIPT') return NodeFilter.FILTER_REJECT; start yielding nodes from inside the pruned subtree instead of failing loudly — sanitizers and text extractors built on TreeWalker silently got extra content. Split the filter into a three-valued `_filter` (keeping `_accept` as the boolean wrapper the sibling/child movers use) and give nextNode a second traversal step for the reject case. That step lives in the DOM layer as `DomTree::next_after_subtree`, mirroring how #447 put `next_in_subtree` there: both share the ancestor climb, and REJECT costs no more JS/native crossings than SKIP. NodeIterator shares this walker but has no subtree pruning (DOM 6.2: FILTER_REJECT behaves as FILTER_SKIP), so `createNodeIterator` marks the walker `_rejectIsSkip` and keeps the plain step. Covered by its own test so the shared implementation cannot drift. --- crates/obscura-dom/src/tree.rs | 44 +++++++++++++++++++++ crates/obscura-js/js/bootstrap.js | 34 ++++++++++++----- crates/obscura-js/src/ops.rs | 9 +++++ crates/obscura-js/src/runtime.rs | 63 +++++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 9 deletions(-) diff --git a/crates/obscura-dom/src/tree.rs b/crates/obscura-dom/src/tree.rs index 725cca7..e428ebf 100644 --- a/crates/obscura-dom/src/tree.rs +++ b/crates/obscura-dom/src/tree.rs @@ -564,7 +564,28 @@ impl DomTree { if let Some(child) = current_node.first_child { return Some(child); } + Self::climb_to_next_sibling(&inner, root, current) + } + /// Returns the node after the whole subtree rooted at `current`, in document + /// order, without leaving the subtree rooted at `root`. + /// + /// This is `next_in_subtree` minus the descend-into-children step, which is + /// what `NodeFilter.FILTER_REJECT` needs: it rejects a node *and* its + /// descendants, unlike `FILTER_SKIP`, which only skips the node itself and + /// is served by `next_in_subtree`. + pub fn next_after_subtree(&self, root: NodeId, current: NodeId) -> Option { + let inner = self.inner.borrow(); + Self::climb_to_next_sibling(&inner, root, current) + } + + /// Follow `current`'s next sibling, climbing ancestors until one has a next + /// sibling — without stepping outside `root`. + fn climb_to_next_sibling( + inner: &DomTreeInner, + root: NodeId, + current: NodeId, + ) -> Option { let mut node_id = current; for _ in 0..=inner.nodes.len() { if node_id == root { @@ -1027,6 +1048,29 @@ mod tests { assert_eq!(tree.next_in_subtree(root, second), None); } + #[test] + fn test_next_after_subtree_skips_descendants() { + // Same shape as above: root > [first > nested, second]. Stepping past + // `first` must land on `second`, not descend into `nested` — that is + // what NodeFilter.FILTER_REJECT needs. + let tree = DomTree::new(); + let root = tree.new_node(NodeData::Text { contents: "root".into() }); + let first = tree.new_node(NodeData::Text { contents: "first".into() }); + let nested = tree.new_node(NodeData::Text { contents: "nested".into() }); + let second = tree.new_node(NodeData::Text { contents: "second".into() }); + tree.append_child(tree.document(), root); + tree.append_child(root, first); + tree.append_child(first, nested); + tree.append_child(root, second); + + assert_eq!(tree.next_after_subtree(root, first), Some(second)); + // A leaf behaves identically to next_in_subtree: there is no subtree. + assert_eq!(tree.next_after_subtree(root, nested), Some(second)); + assert_eq!(tree.next_after_subtree(root, second), None); + // Rejecting the root itself exhausts the walk rather than escaping it. + assert_eq!(tree.next_after_subtree(root, root), None); + } + // Builds a chain of `depth` nested
elements under the document and // returns (outermost_div, innermost_div). Depth this large overflows a // recursive tree walk and aborts the process, so it guards the iterative diff --git a/crates/obscura-js/js/bootstrap.js b/crates/obscura-js/js/bootstrap.js index 1b27126..ff1ca2a 100644 --- a/crates/obscura-js/js/bootstrap.js +++ b/crates/obscura-js/js/bootstrap.js @@ -2616,21 +2616,33 @@ class Document extends Node { currentNode: root, whatToShow: whatToShow, filter: filter || null, - _accept(node) { + // Three-valued per NodeFilter: 1 ACCEPT, 2 REJECT, 3 SKIP. REJECT and + // SKIP both mean "don't return this node", but only REJECT prunes its + // descendants, so nextNode() needs to tell them apart (issue #461). + // A node filtered out by whatToShow is a SKIP: the spec never consults + // the filter for it, and its descendants stay eligible. + _filter(node) { const nodeType = node.nodeType; - const show = (whatToShow >> (nodeType - 1)) & 1; - if (!show) return false; + if (!((whatToShow >> (nodeType - 1)) & 1)) return 3; if (this.filter) { - if (typeof this.filter === 'function') return this.filter(node) === 1; - if (this.filter.acceptNode) return this.filter.acceptNode(node) === 1; + if (typeof this.filter === 'function') return this.filter(node); + if (this.filter.acceptNode) return this.filter.acceptNode(node); } - return true; + return 1; }, + _accept(node) { return this._filter(node) === 1; }, nextNode() { let node = _wrap(+_dom("next_in_subtree", this.root._nid, this.currentNode._nid)); while (node) { - if (this._accept(node)) { this.currentNode = node; return node; } - node = _wrap(+_dom("next_in_subtree", this.root._nid, node._nid)); + const verdict = this._filter(node); + if (verdict === 1) { this.currentNode = node; return node; } + // FILTER_REJECT skips the node AND its subtree; FILTER_SKIP (and any + // other non-accept value) skips only the node. NodeIterator has no + // pruning at all, so `_rejectIsSkip` keeps it on the plain step. + const step = (verdict === 2 && !this._rejectIsSkip) + ? "next_after_subtree" + : "next_in_subtree"; + node = _wrap(+_dom(step, this.root._nid, node._nid)); } return null; }, @@ -2693,7 +2705,11 @@ class Document extends Node { return walker; } createNodeIterator(root, whatToShow, filter) { - return this.createTreeWalker(root, whatToShow, filter); + // Shares the TreeWalker implementation, but DOM 6.2 gives NodeIterator no + // subtree pruning: FILTER_REJECT behaves exactly as FILTER_SKIP there. + const iterator = this.createTreeWalker(root, whatToShow, filter); + iterator._rejectIsSkip = true; + return iterator; } getSelection() { return this.defaultView ? _selectionFor(this) : null; } get activeElement() { return globalThis.__obscura_focused || this.body; } diff --git a/crates/obscura-js/src/ops.rs b/crates/obscura-js/src/ops.rs index c578ac8..aeb4405 100644 --- a/crates/obscura-js/src/ops.rs +++ b/crates/obscura-js/src/ops.rs @@ -275,6 +275,15 @@ fn op_dom_inner(state: &OpState, cmd: String, arg1: String, arg2: String) -> Str .map(|id| id.index().to_string()) .unwrap_or("-1".into()) } + // Step past a whole subtree rather than into it: NodeFilter.FILTER_REJECT + // prunes the rejected node's descendants, unlike FILTER_SKIP. + "next_after_subtree" => { + let root = NodeId::new(arg1.parse::().unwrap_or(0)); + let current = NodeId::new(arg2.parse::().unwrap_or(0)); + dom.next_after_subtree(root, current) + .map(|id| id.index().to_string()) + .unwrap_or("-1".into()) + } "child_nodes" => { let nid = arg1.parse::().unwrap_or(0); let ids: Vec = dom.children(NodeId::new(nid)).iter().map(|id| id.index() as i32).collect(); diff --git a/crates/obscura-js/src/runtime.rs b/crates/obscura-js/src/runtime.rs index 740c0ac..6e2f001 100644 --- a/crates/obscura-js/src/runtime.rs +++ b/crates/obscura-js/src/runtime.rs @@ -1377,6 +1377,69 @@ mod tests { ); } + /// Issue #461: FILTER_REJECT must prune the rejected node's whole subtree, + /// while FILTER_SKIP only skips the node and leaves descendants eligible. + /// Collapsing both into "not accepted" let a TreeWalker yield nodes from + /// inside a subtree the page explicitly rejected. + #[test] + fn tree_walker_filter_reject_prunes_the_whole_subtree() { + let mut rt = setup_runtime( + r#"

deep

"#, + ); + rt.run_page_init(); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + function walk(verdict) { + const w = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, { + acceptNode(node) { + return node.tagName === 'SECTION' ? verdict : NodeFilter.FILTER_ACCEPT; + } + }); + const seen = []; + let node; + while ((node = w.nextNode())) seen.push(node.tagName); + return seen; + } + return [walk(NodeFilter.FILTER_REJECT), walk(NodeFilter.FILTER_SKIP)]; + "#, + ) + .unwrap(); + // REJECT drops

with its

parent; SKIP drops only
. + assert_eq!(result, serde_json::json!([["A"], ["P", "A"]])); + } + + /// Issue #461: NodeIterator has no subtree pruning — DOM 6.2 says + /// FILTER_REJECT behaves as FILTER_SKIP there. The shared walker must not + /// leak TreeWalker's pruning into it. + #[test] + fn node_iterator_treats_filter_reject_as_skip() { + let mut rt = setup_runtime( + r#"

deep

"#, + ); + rt.run_page_init(); + let result = rt + .evaluate( + r#" + const root = document.getElementById('root'); + const it = document.createNodeIterator(root, NodeFilter.SHOW_ELEMENT, { + acceptNode(node) { + return node.tagName === 'SECTION' + ? NodeFilter.FILTER_REJECT + : NodeFilter.FILTER_ACCEPT; + } + }); + const seen = []; + let node; + while ((node = it.nextNode())) seen.push(node.tagName); + return seen; + "#, + ) + .unwrap(); + assert_eq!(result, serde_json::json!(["P", "A"])); + } + #[test] fn append_child_flattens_document_fragment() { let mut rt = setup_runtime(r#"
"#);