Repository navigation
Fix citation tooltip links (#147) and harden against XSS - #161
Merged
Merged
Conversation
Co-authored-by: paladox <paladox@users.noreply.github.com> Fixes #147
Consolidate the three PR #149 changelog bullets into two user-facing entries and drop internal implementation terms (api.parse, wikitextToHtml, jQuery extraction, CSS pseudo-elements). Also removes the leftover '4.0.1' framing from the original PR: this fix ships in 7.0.0, where the entries are already filed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wikitextToHtml() fallback built tooltip HTML from editor-controlled citation text with no escaping, so citation/link-label payloads such as <img src=x onerror=...> or an href attribute breakout executed in the browser of anyone hovering the citation. Remove it entirely. The fallback existed to cover api.parse 'failing on MW 1.45+', but that was a misattribution: the SMW SchemaContentHandler incompatibility broke load.php config delivery (fixed separately in #145 by moving config to BeforePageDisplay), not the action=parse API. api.parse on ordinary citation wikitext is unaffected. The real defect was extraction: when api.parse returns the marker as a top-level node (not nested in mw-parser-output), jQuery's descendant-only find() missed it and fell through to the unsafe parser. - extractParsedHtml() wraps the parsed output in a container and reads it via native innerHTML + querySelector, so the marker is found regardless of wrapping and no <script> in the parsed string can execute. - When parsing yields nothing usable, fall back to mw.html.escape() of the citation text: readable plain text, no injection surface. This path is rare (genuine api.parse failure only). Also escape the citation key (reference) wherever it is built into tooltip markup. It is read from a data attribute and HTML-decoded by jQuery's .data(), so the server-side attribute escaping does not protect the client-side interpolation; a crafted key would otherwise execute in the tooltip title and in the citation-not-found message. The raw value is still used for cache keys and the Ask API query. reference is coerced with String() first: jQuery's .data() returns a number/boolean for numeric- or boolean-looking keys (e.g. "2024"), and mw.html.escape() calls String.prototype.replace, which would throw on a non-string and silently kill the tooltip. mw.html.escape comes from mediawiki.base, which is always loaded, so no new module dependency is required. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve RELEASE-NOTES.md by keeping both the extension.json migration (#158) entries and the tooltip fix entries under the 7.0.0 section. The tooltip resource module is now declared in extension.json (post-#158); the JS fix needs no dependency change since mw.html.escape comes from the always-loaded mediawiki.base.
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
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.
Summary
Restores rendered links in citation tooltips (#147) and hardens an XSS that #149 introduced — which I flagged on that PR (relayed there by @paladox). This branch is the corrected, reviewed version of that work.
Three substantive commits (plus a merge with
master):da0a34c(from Fix tooltip raw wikitext display and CSS bracket bleed #149) — restoreapi.parseso wikilinks render in tooltips again, and scope the[]bracket pseudo-elements so they stop leaking into tooltip popups.The security fix
The #149 fallback (
wikitextToHtml) built tooltip HTML from editor-controlled citation text with no escaping, so payloads like<img src=x onerror=…>or anhrefattribute breakout executed on hover. It was justified as a workaround for "api.parsefailing on MW 1.45+ (SchemaContentHandler)", but that was a misattribution: the SchemaContentHandler bug brokeload.phpconfig delivery (fixed separately in #145 by moving config toBeforePageDisplay), not theaction=parseAPI. The real defect was extraction —$(parsed).find('.scite-api-parse')uses jQuery's descendant-onlyfind(), which misses the marker whenapi.parsereturns it as a top-level node, so it fell through to the unsafe parser.Changes:
wikitextToHtml— no more hand-rolled, unescaped HTML.extractParsedHtml()reads the parsed marker via nativeinnerHTML+querySelector, so it is found regardless ofmw-parser-outputwrapping and no<script>in the parsed string can execute (unlike jQuery.html(), which evaluates scripts via itsappend()fallback).api.parsefailure) rendersmw.html.escape(citationText): readable plain text, no injection surface.reference) everywhere it is built into tooltip markup. It round-trips through adata-attribute that jQuery.data()HTML-decodes, so the server-side attribute escaping does not protect the client-side interpolation. It is coerced withString()first because jQuery.data()returns a number for numeric keys (e.g.2024), andmw.html.escapecallsString.prototype.replace— which would throw and silently kill the tooltip.No new ResourceLoader dependency:
mw.html.escapelives in the always-loadedmediawiki.base.Testing
node --check— clean.mw.html.escape+ jQuery.data()coercion, replicated from core) — 15/15: XSS payloads neutralised; numeric/boolean keys no longer throw.extractParsedHtml's DOM behaviour and the live tooltip flow have not been verified on a running wiki. A manual hover test on MW 1.43/1.45 + SMW is needed before this leaves draft.Fixes #147.