fix: Visual Selector renders scraped selectors as text, not markup

[kah-ja]

What

The Visual Selector displays the selector of the element under the mouse, and static/js/visual-selector.js:256 assigned that string with innerHTML. The string can contain markup from the scraped page, because content_fetchers/res/xpath_element_scraper.js:16 builds it as '//*[@id="' + n.id + '"]' from the page's raw id attribute. An id such as dupe"><img src=x onerror=...> therefore becomes real DOM nodes in the edit page instead of being shown as the selector it is.

Why

Two gates are missing on the way to that display. findUpTag() runs CSS.escape() on the id, but it returns null when the id is not unique on the page (xpath_element_scraper.js:62, :65), and the caller then falls back to getxpath() (:151-160), which interpolates the id verbatim. That is also the only branch that can emit an invalid selector: a double quote cannot be expressed inside a double-quoted xpath literal, so the resulting //*[@id="a"b"] never matches anything either way. At the display end, sibling code in this repo routes untrusted strings through .text() rather than innerHTML, for example static/js/watch-overview.js:454 ($('<span>').text(line).html()) and static/js/realtime.js:217 (.html(" ").text(data.status)).

How

xpath_element_scraper.js takes the //*[@id="..."] shortcut only for ids without ", < or > and builds the positional path (/html/body/div[1]) for the rest, so those elements stay selectable and are addressed by position instead. visual-selector.js sets textContent on the selector display, which also covers element data already stored in elements.deflate from earlier versions.

Left unchanged on purpose: the stored elements.deflate payload and the visual_selector_data endpoint (the display-end fix covers files written by earlier versions), the include_filters branch at xpath_element_scraper.js:203 (operator input, not page content), and browser-steps.js, which puts the same selectors into input values with .val().

Testing

Added test_visual_selector_xpath_carries_no_page_markup to tests/visualselector/test_fetch_data.py, the file the playwright and pyppeteer CI jobs already run. It serves a page with the same id on two elements, fetches it with html_webdriver, and asserts the stored selectors carry no markup while both divs keep a selector and a unique id still uses the #id form. On the unpatched tree it fails with //*[@id="dupe"><img src=x onerror=console.log(1)>"]; with the change the file passes 5/5 against a sockpuppetbrowser container.

Also checked by hand against a running instance watching such a page: the stored selectors become /html/body/div[1] and /html/body/div[2], hovering shows the selector as literal text with no child elements, and #unique-benign and /html/body/h1 display as before. tests/test_security.py passes 17/17.

ruff check on the touched test file reports the same 6 pre-existing findings as on master, no new ones. The full suite was not run, only those two test files.

References

GHSA-8m69-92q9-qcxx (reported privately, unpublished at time of writing)

[dgtlmoon]

ah thanks for this!