Page MenuHomePhabricator
Create Task
Maniphest T298147

Prevent browser extensions from inserting junk into the page HTML that gets saved
Open, Needs TriagePublic
Actions

Description

@Esanders thinks that we can prevent browser extensions from inserting junk into the page HTML that gets saved. Currently, we try to filter it out after it is inserted, based on a long list of rules for each one we've discovered (see T54327, occasionally updated e.g. T297862).

Ed: Aside from this being a never ending of task of whack-a-mole with an unlimited number of browser extensions and versions, we also risk removing real content. Right now a MediaWiki document is unlikely to contain content that matches these selectors, but as the list grows the chances of this happening increase.

Details

SubjectRepoBranchLines +/-
Fix selector for TemplateStyles exclusionmediawiki/extensions/VisualEditormaster+1 -1
Follow-up I420bfcac8: Fix typo in loopmediawiki/extensions/VisualEditormaster+2 -2
Filter <script> tags during parse, instead of savemediawiki/extensions/VisualEditormaster+13 -0
Log whenever browser plugin spam is detectedmediawiki/extensions/VisualEditormaster+12 -1
Add 'error.visualeditor' topicmediawiki/extensions/WikimediaEventsmaster+1 -1
Don't attach DOM nodes from the DM store to the main documentmediawiki/extensions/Citemaster+4 -2
Customize query in gerrit

Related Objects
Search...

Event Timeline

matmarex created this task.Dec 21 2021, 8:31 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 21 2021, 8:31 PM
gerritbot added a comment.Dec 21 2021, 8:32 PM

Change 748842 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):

[mediawiki/extensions/Cite@master] Don't attach DOM nodes from the DM store to the main document

https://gerrit.wikimedia.org/r/748842

gerritbot added a project: Patch-For-Review.Dec 21 2021, 8:32 PM

Change 749173 had a related patch set uploaded (by Bartosz Dziewoński; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Log whenever browser plugin spam is detected

https://gerrit.wikimedia.org/r/749173

matmarex added a subtask: T298106: Drop support for browsers which can't use DOMParser in HTML mode.Dec 21 2021, 8:32 PM
matmarex mentioned this in T297862: "shield-container" <div> added in every <ref>.
matmarex assigned this task to Esanders.Dec 21 2021, 8:37 PM
matmarex added a project: Editing-team (Kanban Board).
matmarex moved this task from To Triage to Triaged on the VisualEditor board.
• ppelberg subscribed.Dec 22 2021, 1:04 AM
Esanders updated the task description. (Show Details)Dec 22 2021, 1:28 PM
matmarex moved this task from Incoming to Code Review on the Editing-team (Kanban Board) board.Dec 22 2021, 5:18 PM
gerritbot added a comment.Dec 22 2021, 10:53 PM

Change 748842 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Don't attach DOM nodes from the DM store to the main document

https://gerrit.wikimedia.org/r/748842

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.16; 2022-01-03).Dec 22 2021, 11:00 PM
• ppelberg closed subtask T298106: Drop support for browsers which can't use DOMParser in HTML mode as Resolved.Dec 23 2021, 1:26 AM
matmarex mentioned this in T298306: References glitching out in VisualEditor.Dec 28 2021, 3:17 PM
gerritbot added a comment.Feb 21 2022, 3:32 PM

Change 764383 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/WikimediaEvents@master] Add 'error.visualeditor' topic

https://gerrit.wikimedia.org/r/764383

gerritbot added a comment.Feb 21 2022, 10:48 PM

Change 749173 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Log whenever browser plugin spam is detected

https://gerrit.wikimedia.org/r/749173

gerritbot added a comment.Feb 21 2022, 11:17 PM

Change 764383 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Add 'error.visualeditor' topic

https://gerrit.wikimedia.org/r/764383

gerritbot added a comment.Feb 21 2022, 11:29 PM

Change 749173 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Log whenever browser plugin spam is detected

https://gerrit.wikimedia.org/r/749173

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.23; 2022-02-21); removed MW-1.38-notes (1.38.0-wmf.16; 2022-01-03).Feb 22 2022, 12:00 AM
matmarex moved this task from Code Review to Doing on the Editing-team (Kanban Board) board.Feb 22 2022, 12:37 AM
matmarex removed a project: Patch-For-Review.

Next step is to wait a few days or weeks, and then check the logs.

  • If we find that we're logging no plugin spam, then we should congratulate Ed for fixing the problem with https://gerrit.wikimedia.org/r/748842, pat ourselves on the back, and remove the old code. (But before we do that, check that logging is working.)
  • If we find that we're logging some plugin spam, then we'll have to go back and think about it some more.
VPuffetMichel moved this task from Doing to Blocked / Needs More Work on the Editing-team (Kanban Board) board.Apr 25 2022, 5:12 PM
• ppelberg moved this task from Blocked / Needs More Work to Ready to Be Worked On on the Editing-team (Kanban Board) board.May 9 2022, 5:16 PM
matmarex added a comment.May 9 2022, 6:18 PM

Logstash query: https://logstash.wikimedia.org/goto/5fd5d9fcaf327d643a93b3e1ac193ec0
Dashboard with some extra info: https://logstash.wikimedia.org/goto/6c2c46cf3af87dedde43ab6d8717374a

image.png (539×2 px, 54 KB)

The problem is not entirely fixed, but now we have some data about what is getting inserted, so maybe we can make some guesses as to how it happens. Or maybe we can just decide that the current approach with the deny list works well enough.

matmarex added a comment.May 9 2022, 6:28 PM

Initial review of the most common entries:
(I'm not pasting the data from Logstash to ensure we don't accidentally leak some private information)

Esanders added a comment.May 10 2022, 4:13 PM

At a quick glance, it looks like the <script> injection is happening at a network level, before the DOM is even parsed. This means my original hypothesis that in-memory DOM objects aren't getting modified may still be true. We could test this by adding a <script>-stripper to our HTML parser.

The Chimera images must be from a bug in our code. I don't think even copy and paste would allow such HTML to end up in the model.

gerritbot added a comment.May 10 2022, 4:31 PM

Change 790723 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Filter <script> tags during parse, instead of save

https://gerrit.wikimedia.org/r/790723

gerritbot added a project: Patch-For-Review.May 10 2022, 4:31 PM
gerritbot added a comment.May 11 2022, 9:52 PM

Change 790723 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Filter <script> tags during parse, instead of save

https://gerrit.wikimedia.org/r/790723

gerritbot added a comment.May 11 2022, 11:21 PM

Change 791100 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Follow-up I420bfcac8: Fix typo in loop

https://gerrit.wikimedia.org/r/791100

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.12; 2022-05-16).May 11 2022, 11:30 PM
gerritbot added a comment.May 11 2022, 11:38 PM

Change 791100 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Follow-up I420bfcac8: Fix typo in loop

https://gerrit.wikimedia.org/r/791100

matmarex moved this task from Ready to Be Worked On to Blocked / Needs More Work on the Editing-team (Kanban Board) board.May 12 2022, 12:23 AM
matmarex removed a project: Patch-For-Review.

Waiting until next deployment to review the logs again.

matmarex moved this task from Blocked / Needs More Work to Ready to Be Worked On on the Editing-team (Kanban Board) board.May 23 2022, 5:08 PM
matmarex removed a project: Editing-team (Kanban Board).Jul 26 2022, 11:52 PM

We're still logging just as many entries as before.

I suggest we drop this for now.

Esanders added a comment.EditedWed, Apr 17, 12:00 PM

I'm not seeing many entries - in the last month there are 77 hits, all a false positives for either:

  • <style data-mw-deduplicate="... (TempateStyles) because we only filter style:not( [ data-mw ] )

or

  • <img role="none" alt="" class="ve-ce-chimera ve-ce-chimera-gecko" src="data:image/gif...
Esanders added a subscriber: dchan.Wed, Apr 17, 1:17 PM

Not sure how ve-ce-chimera's are leaking through to the DM? CC @dchan

gerritbot added a comment.Wed, Apr 17, 1:19 PM

Change #1020829 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Fix selector for TemplateStyles exclusion

https://gerrit.wikimedia.org/r/1020829

gerritbot added a project: Patch-For-Review.Wed, Apr 17, 1:19 PM
gerritbot added a comment.Wed, Apr 17, 5:32 PM

Change #1020829 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Fix selector for TemplateStyles exclusion

https://gerrit.wikimedia.org/r/1020829

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.2; 2024-04-23).Wed, Apr 17, 6:00 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL