Page MenuHomePhabricator

[Regression] Copying references between VE instances does not work
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What should have happened instead?: copy-pasting references should work, even when copying reused references.

This is a regression, it used to work a few months ago.

Other information (browser name/version, screenshots, etc.):
Latest FF on Windows 10

Event Timeline

I can reproduce locally. This seems to happen when the reference uses a citation template, but not when it uses plain text, and regardless of whether or not it is reused.

I bisected it – it's caused by rEVED4bc814f1a055: Update VE core submodule to master (bae9101b7), which contains several changes to copy/paste code.

I've pulled out the functions that were relocated and diffed them separately here: https://phabricator.wikimedia.org/P74141

This bug is caused by a change to DOMPurify: https://github.com/cure53/DOMPurify/commit/ae517d6fb004b29421ae57a00432cfd463805622

/* Work around a security issue with comments inside attributes */
if (SAFE_FOR_XML && regExpTest(/((--!?|])>)|<\/(style|title)/i, value)) {
  _removeAttribute(name, currentNode);
  continue;
}

When we copy from VE inside Romanian wikipedia, we get a ref node wrapping a format node. When we pick up the format node it comes along with a <style> element, and the closing tag gets caught by this sanitiser match, which duly removes the entire data-mw attribute while pasting. This leaves us with an element which has a type indicating that it's a ref, but no data to place within it. That's why the bug only appears on certain wikipedias, and why it only happens when a ref contains another node.

We've a few options from here:

  • Add a DOMPurify hook that specifically allows mw-data
  • Fudge the copying code to escape </style> somehow
  • Fudge the pasting code to escape </style> somehow
  • Avoid copying <style/> tags entirely

DOMPurify changes caused a similar problem some time ago: T279215#8525105 and that time, they resolved it after we reported it.

Or maybe you could turn off the SAFE_FOR_XML option, it was added alongside that change, and it may not be needed for VE. I haven't tried to understand what exactly it does.

Change #1128474 had a related patch set uploaded (by Zoe; author: Zoe):

[VisualEditor/VisualEditor@master] Clipboard: escape certain sequences to avoid DOMPurify removing data-mw

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

Change #1128474 abandoned by Zoe:

[VisualEditor/VisualEditor@master] Clipboard: escape certain sequences to avoid DOMPurify removing data-mw

Reason:

Ed pointed out this should be in the MW parts of the codebase as it's MW-specific.

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

Change #1129852 had a related patch set uploaded (by Zoe; author: Zoe):

[mediawiki/extensions/Cite@master] Ensure data-mw attribute isn't removed by DOMPurify on paste

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

Change #1129852 merged by jenkins-bot:

[mediawiki/extensions/Cite@master] Ensure data-mw attribute isn't removed by DOMPurify on paste

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