Last modified: 2014-03-04 18:17:37 UTC
Extension: https://www.mediawiki.org/wiki/Extension:Popups Gerrit: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/Popups
Instances of `.html()` - 1. In the `createBox` method I do something like: $el.html( $el.html() ); Its to refresh the DOM and display the SVG elements (see comments in the method) that were added in the `createThumbnail` method. The elements created there follow [1] and thus are escaped. 2. To create the SVG element that masks the popup to create the triangle I do: $svg.html( '<svg width="0" height="0">...</svg>' ); Making this through jQuery methods was becoming to verbose. This a plain string with no concatenation from anywhere so I guess its safe. 3. There is an i18n string if the page redirects, it needs to read like "redirects to OtherPage". As in certain languages it could be "OtherPage…" and not "…OtherPage", Mark suggested that I add a $1 to it [2]. As I need those elements to be styled a certain way, the i18n strings will end up having an <h3> and thus my code looks something like this $( '<div>' ).html( mw.message( 'popups-redirects', redirects[ 0 ].to ).text() ); I am not sure if this is safe. [1] https://www.mediawiki.org/wiki/Manual:Coding_conventions/JavaScript#Creating_elements [2] https://gerrit.wikimedia.org/r/#/c/111983/3/Popups.i18n.php
(In reply to Prateek Saxena from comment #1) > Instances of `.html()` - > > 1. In the `createBox` method I do something like: > > $el.html( $el.html() ); > > Its to refresh the DOM and display the SVG elements (see comments in the > method) that were added in the `createThumbnail` method. The elements > created there follow [1] and thus are escaped. > I'm mostly concerned about the $contentbox portion, since that is generated from user content. > > 2. To create the SVG element that masks the popup to create the triangle I > do: > > $svg.html( '<svg width="0" height="0">...</svg>' ); > > Making this through jQuery methods was becoming to verbose. This a plain > string with no concatenation from anywhere so I guess its safe. Yes, this part is fine. > 3. There is an i18n string if the page redirects, it needs to read like > "redirects to OtherPage". As in certain languages it could be "OtherPage…" > and not "…OtherPage", Mark suggested that I add a $1 to it [2]. As I need > those elements to be styled a certain way, the i18n strings will end up > having an <h3> and thus my code looks something like this > > $( '<div>' ).html( mw.message( 'popups-redirects', redirects[ 0 ].to > ).text() ); > > I am not sure if this is safe. > > [1] > https://www.mediawiki.org/wiki/Manual:Coding_conventions/ > JavaScript#Creating_elements > [2] https://gerrit.wikimedia.org/r/#/c/111983/3/Popups.i18n.php Is there a working version of this in labs somewhere that I can test with? Or can you list out what dependencies this has? I'm not able to get it working locally.
(In reply to Chris Steipp from comment #2) > I'm mostly concerned about the $contentbox portion, since that is generated > from user content. We are using .text() when placing the extract in the Popup[1]. Are there any other measures that need to be taken? The other elements are being created in jQuery (how the code convention link explains) > Yes, this part is fine. Alright! > Is there a working version of this in labs somewhere that I can test with? > Or can you list out what dependencies this has? I'm not able to get it > working locally. There is a test instance[2] where the latest code lives. A couple of people have had the same issue and I am not sure what is wrong. I'll talk to Yuvi and resolve this. Are you using the vagrant role (popups) to set it up? [1] https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FPopups/2b021ef048aac6bfcbd0c1944bccc9ba2d7db040/resources%2Fext.popups.core.js#L53 [2] http://chicken.wmflabs.org/wiki/TestNavPopUps
(In reply to Prateek Saxena from comment #3) > (In reply to Chris Steipp from comment #2) > > I'm mostly concerned about the $contentbox portion, since that is generated > > from user content. > > We are using .text() when placing the extract in the Popup[1]. Are there any > other measures that need to be taken? The other elements are being created > in jQuery (how the code convention link explains) No, .text() doesn't stop several attacks. For example: $i = $( "<div>asdf<script>alert(1)</script></div>" ); $o = $( "<div/>" ); $o.html( $i.text() ); You may be able to santize it with mw.html.escape, but I'm not entirely sure what markup you're trying to pass through. > > Yes, this part is fine. > > Alright! > > > > Is there a working version of this in labs somewhere that I can test with? > > Or can you list out what dependencies this has? I'm not able to get it > > working locally. > > There is a test instance[2] where the latest code lives. A couple of people > have had the same issue and I am not sure what is wrong. I'll talk to Yuvi > and resolve this. Are you using the vagrant role (popups) to set it up? As soon as I install it, ResourceLoader complains that it can't find the class ResourceLoaderSchemaModule. I'm not sure if that's a typo, or if you're pulling that in from another extension. > > [1] > https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FPopups/ > 2b021ef048aac6bfcbd0c1944bccc9ba2d7db040/resources%2Fext.popups.core.js#L53 > [2] http://chicken.wmflabs.org/wiki/TestNavPopUps
(In reply to Chris Steipp from comment #4) > > We are using .text() when placing the extract in the Popup[1]. Are there any > > other measures that need to be taken? The other elements are being created > > in jQuery (how the code convention link explains) > > No, .text() doesn't stop several attacks. For example: > $i = $( "<div>asdf<script>alert(1)</script></div>" ); > $o = $( "<div/>" ); > $o.html( $i.text() ); > > You may be able to santize it with mw.html.escape, but I'm not entirely sure > what markup you're trying to pass through. Sorry, I should have looked at your reference first. Yeah, setting the text like that should work for that case. I'm digging through the TextExtracts section to make sure it can't return anything else dangerous.
(In reply to Chris Steipp from comment #5) > Sorry, I should have looked at your reference first. Yeah, setting the text > like that should work for that case. I'm digging through the TextExtracts > section to make sure it can't return anything else dangerous. Ok, it should be fine as is. It would be helpful for you to document around the lines where you do .text( page.extract ) and .html( $box.html() ) what the expectations are, so that someone doesn't change those in the future and open up an issue. (In reply to Prateek Saxena from comment #1) > 3. There is an i18n string if the page redirects, it needs to read like > "redirects to OtherPage". As in certain languages it could be "OtherPage…" > and not "…OtherPage", Mark suggested that I add a $1 to it [2]. As I need > those elements to be styled a certain way, the i18n strings will end up > having an <h3> and thus my code looks something like this > > $( '<div>' ).html( mw.message( 'popups-redirects', redirects[ 0 ].to > ).text() ); > > I am not sure if this is safe. The title should be fine, due to the page naming rules. If the message contains scripts, that would cause an issue, but we accept that risk in many places in MediaWiki, so this isn't much different. So in general, as if 2b021ef, this extension looks ok for security. Ori should review it for performance next.
