Page MenuHomePhabricator

Possible problems of loading noscript contents as text when lazy loading images
Closed, ResolvedPublic

Description

In https://gerrit.wikimedia.org/r/#/c/268185/16/resources/mobile.startup/Skin.js @Krinkle mentioned:

Related to $image = $( $noscript.text() ),

This is wrong. The html is not html-escaped from the server (and shouldn't since otherwise it won't work as html for noscript clients). Which means this should be read as html, not text. Otherwise this allows freeform text to become arbitrary html. Aside from a security issue, it will also break some urls containing special characters by wrongly unescaping them.

@Jhernandez response:

Are you suggesting that using $().text will bring problems?
Using .html() to read the contents of the <noscript/> tag returns the html string escaped: "&lt;img alt="Alicante en medi..." which won't build the DOM nodes in $.parseHTML, just give back a text node (which I would have to get the textContent of too.
It would look something like this:

$image = $( $.parseHTML( $.parseHTML( $noscript.html() )[0].textContent ) ),

Which looks like a nightmare and would have to be bounds checked.

What are the problems of getting the noscript contents with .text()? Should we use .html()?

Related Objects

StatusSubtypeAssignedTask
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolveddr0ptp4kt
DuplicateJhernandez
Duplicatedr0ptp4kt
DeclinedNone
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedBBlack
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJhernandez
ResolvedJhernandez
ResolvedBBlack
ResolvedNuria

Event Timeline

Jhernandez raised the priority of this task from to Needs Triage.
Jhernandez updated the task description. (Show Details)
Jhernandez added a project: MobileFrontend.
Jdlrobson triaged this task as Medium priority.Feb 11 2016, 4:56 PM
Jdlrobson added a subscriber: Jdlrobson.

I don't see why one would need to read the <noscript> element at all. Any relevant information should already be readily available in the DOM to read normally from the surrounding or wrapping element.

Compare to existing lazy-load implementations, both open-source and proprietary. I've never seen it done this way and don't see why we would.

Jdlrobson renamed this task from Spike: Possible problems of loading noscript contents as text when lazy loading images to Possible problems of loading noscript contents as text when lazy loading images.Feb 29 2016, 5:27 PM

Change 275670 had a related patch set uploaded (by Jdlrobson):
Use the src attribute Luke

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

Change 275670 merged by jenkins-bot:
Use the src attribute Luke

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