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

StatusAssignedTask
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
DeclinedNone
OpenNone
Resolveddr0ptp4kt
DuplicateJhernandez
Duplicatedr0ptp4kt
OpenNone
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 Normal priority.Feb 11 2016, 4:56 PM
Jdlrobson added a subscriber: Jdlrobson.
Jdlrobson moved this task from Backlog to Tasks on the MobileFrontend board.Feb 18 2016, 6:25 PM

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
Jdlrobson claimed this task.Mar 7 2016, 8:55 PM
Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-67-If, Then, Else...? board.

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

Jdlrobson closed this task as Resolved.Mar 10 2016, 6:40 PM