Escape CSS idents - #358
Open
kepano wants to merge 4 commits into
Open
Conversation
getElementSelector() interpolated an element's id into a selector without escaping, while the adjacent class branch was already guarded. React streaming SSR emits Suspense boundaries as `id="S:a"` / `id="B:0"`, so the generated `div#S:a` made css-select parse `:a` as a pseudo-class and throw. The throw was caught by parseInternal's own handler, which returns the whole <body> via _serializeFallbackBody(). Because that fallback is word-count compared against the real extraction and a body dump always out-words a thin one, callers saw no exception and no empty string — just a result roughly 2x larger and full of nav/footer chrome. Adds escapeCssIdent() to utils/dom.ts alongside escapeHtml/isDangerousUrl. It implements the CSSOM CSS.escape algorithm rather than delegating to CSS.escape where present: linkedom has no CSS global, so Node/CLI/Worker need it anyway, and always using it keeps selectors identical across environments. Verified byte-identical to the spec algorithm across 31 ids, including the leading-digit and lone-hyphen cases, and confirmed each round-trips through linkedom's querySelector. Also guards the querySelector call on options.contentSelector, which is public API input and may be unparseable from the caller. It now degrades to auto-detection instead of failing the parse.
When any pipeline step throws, parseInternal() catches it and returns the
entire <body> via _serializeFallbackBody(). That result was structurally
indistinguishable from a real extraction — same shape, same wordCount field,
no marker — so nothing downstream could reason about it.
Because a body dump counts nav, sidebars and footer, its word count beats a
real extraction of the same page. Two consequences, both live:
- a degraded first parse has an inflated count that satisfies the < 200 and
< 50 retry gates, so the retries that would find real content never run
- a degraded retry wins whichever comparison it was entered into
parse() had five acceptance decisions with hand-rolled predicates (> 2x,
> 2x, a compound focus test, >, and the schema.org path with no comparison
at all), and no shared notion of what makes a candidate acceptable.
Marks fallback results in a WeakSet, keeping the marker out of
DefuddleResponse so it never reaches callers. Adds _effectiveWordCount(),
which counts a degraded result as zero for gating, and _preferCandidate(),
one acceptance helper the five sites now share: degraded candidates never
win, any real extraction beats a degraded current, otherwise the site's own
predicate decides. The existing predicates are unchanged.
The schema.org path keeps its "trust schema" semantics rather than gaining a
word-count comparison — that would change behaviour on healthy pages, and
this is meant to affect only the failure path. It now rejects a degraded
retry, which was the actual bug.
Two propagation details: the extractor pipeline spreads pipelineResult into a
new object, losing the marker, so it is carried across; and the two fallback
call sites were near-identical blocks, now a single _degradedResponse().
Verified by forcing standardizeContent to throw on the first parse only.
Before: 274 words with nav and footer leaked. After: 256 clean.
standardizeFootnotes built `a[id="${id}"]` from an id read off the page. A
double quote or backslash in that id closed the attribute selector early, so
css-select threw "Attribute selector didn't terminate" inside the footnote
pass — which parseInternal catches, discarding the whole extraction in favour
of the <body> fallback.
Same class of bug as the id escaping in 9f11c9b, but it needs no escaping:
the correct idiom was already four lines below, where a[name] is matched with
querySelector('a[name]') plus a getAttribute comparison in JS. This does the
same for a[id], keeping the case-sensitive exact match the selector had.
Fixture has two definitions because generic footnote detection requires at
least two candidate references before it runs. Without the fix its expected
output leaks raw <sup><a href="#fn"1"> markup into the markdown and the
definitions stay an ordinary numbered list instead of becoming [^1] / [^2].
The helper is exported from utils/dom.ts and had no committed coverage. It implements the full CSSOM CSS.escape algorithm rather than a shorter approximation, and the branches most likely to look redundant are exactly the ones that matter: a leading digit, a digit after a leading hyphen, and a lone hyphen all require the hex-escape form and cannot be written as an inline backslash escape. Those are pinned explicitly so the reasoning survives. Cross-checks every id against a verbatim copy of the spec algorithm, so a future rewrite of the helper has an independent oracle to disagree with, and round-trips each escaped id through querySelector to confirm it both parses and selects the intended element. Also covers debug.contentSelector, which is public output that callers paste back in as the contentSelector option and so has to be a selector querySelector accepts. The round-trip list drops three ids under DOM=jsdom. nwsapi does not match the escapes emitting a backslash before a quote or backslash (\" \' \\) in an ID selector, though it accepts their hex forms and browsers accept both; linkedom matches all of them. The escaping is spec-correct either way, which the agreement test covers for those ids regardless of DOM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #352
getElementSelector()interpolated element ids into CSS selectors unescaped. React streaming SSR emitsid="S:a", so the generateddiv#S:aparsed:aas a pseudo-class and threw.parseInternalcaught it and returned the whole<body>— which then won the retry comparison, because that comparison is word-count based and a body dump always out-words a real extraction. Callers saw no exception and no empty string, just a silently doubled result full of nav and footer.Changes
escapeCssIdent()inutils/dom.ts(CSSOMCSS.escapealgorithm — linkedom has noCSSglobal)<body>fallbacks so they can't win a retry comparison or suppress the gates that trigger retries. Replaces five ad-hoc acceptance predicates with one shared helpera[id="${id}"]— the one remaining selector built from a page-derived value, which threw on a quote