[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 --- Comment #4 from Chris Steipp cste...@wikimedia.org --- (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 = $( divasdflt;scriptgt;alert(1)lt;/scriptgt/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 -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 --- Comment #5 from Chris Steipp cste...@wikimedia.org --- (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 = $( divasdflt;scriptgt;alert(1)lt;/scriptgt/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. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 Chris Steipp cste...@wikimedia.org changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #6 from Chris Steipp cste...@wikimedia.org --- (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. -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 --- Comment #2 from Chris Steipp cste...@wikimedia.org --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 Chris Steipp cste...@wikimedia.org changed: What|Removed |Added Assignee|wikibugs-l@lists.wikimedia. |cste...@wikimedia.org |org | -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 --- Comment #3 from Prateek Saxena psax...@wikimedia.org --- (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 -- You are receiving this mail because: You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 Andre Klapper aklap...@wikimedia.org changed: What|Removed |Added Priority|Unprioritized |Normal -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 Andre Klapper aklap...@wikimedia.org changed: What|Removed |Added CC||psax...@wikimedia.org, ||yuvipa...@gmail.com Component|Extension setup |Popups Product|Wikimedia |MediaWiki extensions -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 --- Comment #1 from Prateek Saxena psax...@wikimedia.org --- 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 -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
[Bug 61743] Security review for 'Popups' Extension
https://bugzilla.wikimedia.org/show_bug.cgi?id=61743 Prateek Saxena psax...@wikimedia.org changed: What|Removed |Added Blocks||61167 -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. ___ Wikibugs-l mailing list Wikibugs-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
