PoC clone node#1897
Conversation
✅ Deploy Preview for content-scope-scripts ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Temporary Branch UpdateThe temporary branch has been updated with the latest changes. Below are the details:
Please use the above install command to update to the latest version. |
[Beta] Generated file diffTime updated: Fri, 22 May 2026 10:16:59 GMT AndroidFile has changed AppleFile has changed Chrome-mv3File has changed FirefoxFile has changed IntegrationFile has changed WindowsFile has changed |
* Migrate element hiding integration tests from privacy-test-pages (#2144) * migrate element hiding integration tests from privacy-test-pages * show results at top of page once tests are complete * Refactor renderResults and remove unnecessary peer dependencies Co-authored-by: jkingston <jkingston@duckduckgo.com> * Refactor renderResults to simplify options and improve container handling Co-authored-by: jkingston <jkingston@duckduckgo.com> * Refactor: Keep test summary and results together Co-authored-by: jkingston <jkingston@duckduckgo.com> * Refactor renderResults to use results-container or body Co-authored-by: jkingston <jkingston@duckduckgo.com> * Add element hiding test page and config Co-authored-by: jkingston <jkingston@duckduckgo.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: jkingston <jkingston@duckduckgo.com> * PoC clone node * Update element-hiding.js * Update element-hiding.js * Update element-hiding.js * Lint fix * Update element-hiding.js * Refactor: Improve element hiding logic with DOMParser and custom element handling Co-authored-by: jkingston <jkingston@duckduckgo.com> * feat: Cache custom elements check in element-hiding Co-authored-by: jkingston <jkingston@duckduckgo.com> --------- Co-authored-by: David Harbage <dave@duckduckgo.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: jkingston <jkingston@duckduckgo.com>
Co-authored-by: jkingston <jkingston@duckduckgo.com>
Build Branch
Static preview entry points
QR codes (mobile preview)
Integration commandsnpm (Android / Extension): Swift Package Manager (Apple): .package(url: "https://github.com/duckduckgo/content-scope-scripts.git", branch: "pr-releases/jkt/clone-node-element-hiding")git submodule (Windows): git -C submodules/content-scope-scripts fetch origin pr-releases/jkt/clone-node-element-hiding
git -C submodules/content-scope-scripts checkout origin/pr-releases/jkt/clone-node-element-hidingPin to exact commitnpm (Android / Extension): Swift Package Manager (Apple): .package(url: "https://github.com/duckduckgo/content-scope-scripts.git", revision: "9c4958ffe4703ddb759b21f99b24f394194c5fe1")git submodule (Windows): git -C submodules/content-scope-scripts fetch origin pr-releases/jkt/clone-node-element-hiding
git -C submodules/content-scope-scripts checkout 9c4958ffe4703ddb759b21f99b24f394194c5fe1 |
|
This PR requires a manual review and approval from a member of one of the following teams:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Redundant per-node custom element check adds unnecessary overhead
- Removed the redundant per-node hasCustomElements(node) fallback in isDomNodeEmpty, since the cached useDOMParser value (set from document.body at pass start) already covers all descendant nodes.
Or push these changes by commenting:
@cursor push c4f91ed857
Preview (c4f91ed857)
diff --git a/injected/src/features/element-hiding.js b/injected/src/features/element-hiding.js
--- a/injected/src/features/element-hiding.js
+++ b/injected/src/features/element-hiding.js
@@ -60,7 +60,7 @@
let appliedRules = new Set();
let shouldInjectStyleTag = false;
let styleTagInjected = false;
-/** @type {boolean} Cached per-pass; falls back to per-node checks when false. */
+/** @type {boolean} Cached per-pass via hasCustomElements(document.body). */
let useDOMParser = false;
let mediaAndFormSelectors = 'video,canvas,embed,object,audio,map,form,input,textarea,select,option,button';
let hideTimeouts = [0, 100, 300, 500, 1000, 2000, 3000];
@@ -218,12 +218,10 @@
// Use cloneNode for performance, but fall back to DOMParser when custom elements
// are present to avoid triggering custom element constructors (page-observable).
- // useDOMParser is cached per-pass via hasCustomElements check in hideAdNodes/unhideLoadedAds.
- // If the page-level cache is false, still check the node before cloning.
- const shouldUseDOMParser = useDOMParser || hasCustomElements(node);
+ // useDOMParser is cached per-pass via hasCustomElements(document.body) in hideAdNodes/unhideLoadedAds.
/** @type {HTMLElement} */
let parsedNode;
- if (shouldUseDOMParser) {
+ if (useDOMParser) {
// DOMParser wraps content in <html><head>...</head><body>...</body></html>
if (!parser) {
parser = new DOMParser();| // are present to avoid triggering custom element constructors (page-observable). | ||
| // useDOMParser is cached per-pass via hasCustomElements check in hideAdNodes/unhideLoadedAds. | ||
| // If the page-level cache is false, still check the node before cloning. | ||
| const shouldUseDOMParser = useDOMParser || hasCustomElements(node); |
There was a problem hiding this comment.
Redundant per-node custom element check adds unnecessary overhead
Low Severity
The per-node hasCustomElements(node) fallback in isDomNodeEmpty is logically redundant. When useDOMParser is false (set by hasCustomElements(document.body) at the start of each pass), no node within document.body can contain custom elements, so the per-node check always returns false. This adds unnecessary getElementsByTagName('*') traversal for every element processed — particularly costly for closest-empty rules that walk up the DOM tree, calling hasCustomElements on progressively larger parent subtrees.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
There was a problem hiding this comment.
Please fix this.
There was a problem hiding this comment.
Web Compatibility Assessment
injected/src/features/element-hiding.js:353,372- warning: the new custom-element pre-scan assumesdocument.bodyexists. On body-less documents such as SVG/XML, the scheduled hide/unhide passes throw before rule processing; the previous path only requireddocument.querySelectorAll.
Security Assessment
injected/src/features/element-hiding.js:229- error:DOMParseris now read lazily during timeout-driven rule passes, after page script can replacewindow.DOMParser. That lets a hostile page controlparseFromStringor the returned document and bypass/break element hiding.injected/src/features/element-hiding.js:233- error:node.cloneNode(true)calls the live method on a page-controlled node/prototype. A page can replaceNode.prototype.cloneNodeor an owncloneNodebefore these timers run, executing page code in the protection path or returning an object that subverts the heuristic.
Risk Level
High Risk: this changes DOM-processing behavior for the injected elementHiding feature across platforms and introduces delayed calls to mutable page-world DOM primitives.
Recommendations
- Capture the native
DOMParserconstructor andNode.prototype.cloneNodeat module load, or add safe captured/bound references incaptured-globals.js; invoke prototype methods with capturedReflectApply. - Make
hasCustomElementsuse captured DOM/String primitives if it remains security-relevant, or keep the DOMParser path when safe native capture is not available. - Guard
document.bodyin both hide/unhide passes, or usedocument.documentElement/skip non-HTML documents explicitly. - Add hostile-page tests that replace
window.DOMParser,Node.prototype.cloneNode, and the custom-element traversal primitive beforeinit(), plus a body-less document test.
Sent by Cursor Automation: Web compat and sec
| if (shouldUseDOMParser) { | ||
| // DOMParser wraps content in <html><head>...</head><body>...</body></html> | ||
| if (!parser) { | ||
| parser = new DOMParser(); |
There was a problem hiding this comment.
This now reads DOMParser lazily inside the timeout-driven pass. Since init() runs after config and these timers fire after page script can mutate globals, a hostile page can replace window.DOMParser and control parseFromString or the returned document, making element hiding throw or bypass. The previous module-scope parser at least captured the constructor at injection time. Please capture DOMParser/parseFromString at module load, or add a captured reference in captured-globals.js, before lazy initialization.
| } | ||
| parsedNode = parser.parseFromString(node.outerHTML, 'text/html').documentElement; | ||
| } else { | ||
| parsedNode = /** @type {HTMLElement} */ (node.cloneNode(true)); |
There was a problem hiding this comment.
This calls the live cloneNode property on a page-controlled node. Pages can replace Node.prototype.cloneNode or define an own cloneNode before these timers run, so they can execute page code in the protection path or return an object that breaks the emptiness heuristic. If the clone path stays, capture Node.prototype.cloneNode early and invoke it with captured ReflectApply; the custom-element detector should similarly avoid live page-mutated DOM methods.
| const document = globalThis.document; | ||
|
|
||
| // Cache custom elements check once per pass to avoid repeated DOM traversal | ||
| useDOMParser = hasCustomElements(document.body); |
There was a problem hiding this comment.
This assumes document.body exists. On non-HTML/body-less documents such as SVG or XML, hasCustomElements(document.body) throws before any rule processing, whereas the previous path only needed document.querySelectorAll. Please guard document.body or use an explicit document.documentElement/non-HTML skip path, and mirror the same fix in unhideLoadedAds().
3 participants
Asana Task/Github Issue: https://app.asana.com/1/137249556945/project/1201520793593668/task/1211051310695039?focus=true
Description
Testing Steps
Checklist
Please tick all that apply:
Note
Medium Risk
Changes the core
isDomNodeEmptyheuristic used to hide/unhide elements by switching parsing strategy and adding custom-element detection, which could affect page layout/false positives and has some web-compat risk.Overview
Improves element-empty detection in
element-hidingby preferringcloneNode(true)overDOMParserfor performance, while adding a custom element (hyphenated tag) check to fall back toDOMParserto avoid page-observable custom element constructor side effects.Updates the emptiness heuristic to correctly consider when the root node itself is a media/form element or an
iframe(not just descendants), and caches the custom-element presence check once per hide/unhide pass via a newuseDOMParserflag.Reviewed by Cursor Bugbot for commit e0eb7e8. Bugbot is set up for automated code reviews on this repo. Configure here.