Page MenuHomePhabricator

Bad rendering of map links associated map frames in on enwikivoyage (Parsoid)
Closed, ResolvedPublicBUG REPORT

Description

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

Open https://en.wikivoyage.org/wiki/Paris?useparsoid=1 and look at the first "Map of Paris".

What happens?:

Below the map, there's a marker "'"UNIQ--maplink-00000004-QINU"'".
When opening the map in large format (by double-clicking on it), the title of the map is "'"UNIQ--maplink-00000004-QINU"' Map of Paris".

What should have happened instead?:

The marker should be replaced by a maplink to open the map in full page. The title of the full page map should display "48°51′28″N 2°20′24″E Map of Paris" instead of the marker.

Note
There's a non-zero probability this is related to T345568, but it seems to deserve its own investigation to either update that bug or to keep this one as a stand-alone.
After investigation, it seems related to StripMarkers and legacy template preprocessing.

Event Timeline

Change 961849 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/core@master] [DNM] strip state from attributes before inserting them

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

MSantos added subscribers: sbassett, Bawolff, MSantos.

@sbassett and @Bawolff what's the proper process to request a security review on a specific patch (the same in this phab task)?

@sbassett and @Bawolff what's the proper process to request a security review on a specific patch (the same in this phab task)?

Well, there isn't really one, aside from pinging or cc'ing folks on the Security-Team to have a look, or a security-conscious volunteer like @Bawolff.

I don't see any security implications since one could create the exact same tag as the new version of parser creates in the previous version with the direct xml syntax already. And in any case, afaict this is PST only.

The only thing that came to mind is if running unstripBoth on the attribute could somehow allow lua to peak behind strip markers, but that seems impossible since the whole thing is behind a strip marker anyways.

Thanks! It looked like this was safe but I figured we would better check with you since you had written that previous code. This is not PST only btw. For Parsoid, this issue is triggered during preprocessing.

In any case, I am going to promote my +1 to a +2.

Change 961849 merged by jenkins-bot:

[mediawiki/core@master] Strip state from attributes before inserting them

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

Resolved with the last train on group 1 on enwikivoyage.

Reopening because there is one one other issue I am noticing. Check https://en.wikivoyage.org/wiki/Jenin?useparsoid=1 vs https://en.wikivoyage.org/wiki/Jenin and see the diff in the icon in the two cases. If you inspect, the issues seems to be that the content of the link is -number-around in Parsoid vs 0°0′0″N 0°0′0″E in legacy rendering.

Change 969333 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/extensions/Kartographer@master] Make ParsoidTagHandler stateless

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

Change 969380 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] [POC] Split stateful/stateless code in ParsoidTagHandler

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

Change 969333 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Make ParsoidTagHandler stateless

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

Change 969380 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Split stateful/stateless code in ParsoidTagHandler

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