strip markers can be used to get around html attribute escaping in (many?) parser tags
Closed, ResolvedPublic

Description

Similar to T110128 (And the more older T37315).

Lots of parser tag extensions, take user input. Do something with it, then return escaped html (Which is considered half-serialized html by the parser [usually. You're allowed to return an array to specify otherwise, but I've never seen anyone do that]). If using normal functions to escape the html (Html/XML class, htmlspecialchars, etc), they will think the \x7fUNIQ... string is fine and pass it on. The parser will then replace that string. If the contents of the strip marker aren't safe for an attribute (Rather common. That's kind of the point of strip markers - to protect things from sanitization) than this can lead to XSS.

Example:

<inputbox>
default={{#tag:poem|" autofocus onfocus="alert(1)" f=|compact=true|class=foo=}}
type=create
</inputbox>

There are many extensions that follow the pattern that inputbox does in terms of how it escapes things.

There are probably extensions (And possibly things in core) that can be used in place of poem for generating unsafe in attributes strip markers. (Poem is really not doing anything wrong here)

Note that normal html tags aren't affected since line 1240 of the parser instructs the sanitizer to do unstrip before escaping attributes.

Suggested fix: Unclear, but good first step would probably to start treating U+7F as a bad character to be removed whenever attributes are escaped (e.g. at the bottom of Html::expandAttributes and similar functions).

There are a very large number of changes, so older changes are hidden. Show Older Changes

Another similar example:

{{Special:Contributions|target={{#tag:poem|" autofocus onfocus="alert('XSS')" f=|class=fred=|compact=true}}}}

A variation on the last one that doesn't require poem extension, so it will on a vanilla MW with no extensions installed.

{{Special:Contributions|target={{#tag:gallery||class=autofocus onfocus=alert('XSS') fred=}} }}
csteipp triaged this task as "High" priority.Aug 25 2015, 5:27 PM
csteipp added a subscriber: csteipp.

Thanks for calling this out @Bawolff.

Suggested fix: Unclear, but good first step would probably to start treating U+7F as a bad character to be removed whenever attributes are escaped (e.g. at the bottom of Html::expandAttributes and similar functions).

Definitely a start. We should see how many things that breaks.

Krenair changed the title from "strip markers can be used to get around html attribute escaping in (many?) parser tag extensions" to "strip markers can be used to get around html attribute escaping in (many?) parser tags".Sep 13 2015, 5:09 PM
Bawolff claimed this task.Oct 20 2015, 9:22 PM

Here's my patch for this (I can upload as an attachment if need be, I'm just a little worried I'll accidentally make the upload public by accident):

commit 4d841381040c84728184d1a2231592fc64815f82
Author: Brian Wolff <bawolff+wn@gmail.com>
Date:   Tue Oct 20 16:18:03 2015 -0600

    Make MW attribue escaping methods mangle U+7F to disable strip markers
    
    If you can get a strip marker in an attribute, then you can get an
    XSS by putting an unescaped quote into a strip marker, bypassing
    the html escaping routines until unstrip is called later.
    
    Ideally, user controlled attributes would have unstripBoth()
    run on them before escaping, but there's a couple places that is
    not true (e.g. transcluded special pages), and for better safety,
    we should be treating U+7F as a dangerous character.
    
    This also adds a helper method to the parser, that extensions can
    call to safely escape an html attribute.
    
    Change-Id: I88e808a57b854bb86b2dbdf93299f282c723e75e

diff --git a/includes/Html.php b/includes/Html.php
index c61dca8..7ee7533 100644
--- a/includes/Html.php
+++ b/includes/Html.php
@@ -604,13 +604,19 @@ class Html {
 				// The only difference between this transform and the one by
 				// Sanitizer::encodeAttribute() is '<' is only encoded here if
 				// $wgWellFormedXml is set, and ' is not encoded.
+				// \x7F is modified as it might be part of a strip marker. If
+				// a strip marker got into an attribute, and then later put
+				// through the parser, it might later get replaced with an
+				// unescaped double quote. Ideally though, any strip markers
+				// would be unstripped before reaching here.
 				$map = array(
 					'&' => '&amp;',
 					'"' => '&quot;',
 					'>' => '&gt;',
 					"\n" => '&#10;',
 					"\r" => '&#13;',
-					"\t" => '&#9;'
+					"\t" => '&#9;',
+					"\x7F" => '␡'
 				);
 				if ( $wgWellFormedXml ) {
 					// This is allowed per spec: <http://www.w3.org/TR/xml/#NT-AttValue>
diff --git a/includes/Sanitizer.php b/includes/Sanitizer.php
index f88dd05..5ad27c0 100644
--- a/includes/Sanitizer.php
+++ b/includes/Sanitizer.php
@@ -1050,6 +1050,7 @@ class Sanitizer {
 			"\n" => '&#10;',
 			"\r" => '&#13;',
 			"\t" => '&#9;',
+			"\x7F" => '␡', // disable strip markers
 		) );
 
 		return $encValue;
diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php
index efad151..a70c9ff 100644
--- a/includes/parser/Parser.php
+++ b/includes/parser/Parser.php
@@ -5097,6 +5097,10 @@ class Parser {
 	 * Transform and return $text. Use $parser for any required context, e.g. use
 	 * $parser->getTitle() and $parser->getOptions() not $wgTitle or $wgOut->mParserOptions
 	 *
+	 * If outputting any html attributes, use $parser->escapeAttribute( $text )
+	 * to escape them, not htmlspecialchars! The character \x7F (Ascii delete) is used
+	 * internally by the parser and is not safe to include in html attributes.
+	 *
 	 * Hooks may return extended information by returning an array, of which the
 	 * first numbered element (index 0) must be the return string, and all other
 	 * entries are extracted into local variables within an internal function
@@ -5782,6 +5786,26 @@ class Parser {
 	}
 
 	/**
+	 * Escapes html for an attribute, including substituting strip markers.
+	 *
+	 * Tag extensions (or anything else returning half serialized parser output)
+	 * should use this method to escape attributes. The character \x7F is not safe
+	 * inside the attributes as it might be a strip marker that is later substituted
+	 * for a string, which could contains an unescaped quote mark. Hence this method
+	 * which ensures that strip markers get replaced before escaping.
+	 *
+	 * @note If for some reason you can't use this method, be sure to make sure \x7F
+	 *  gets replaced during your escaping for attributes.
+	 * @param $text String Text to escape
+	 * @return String The escaped attribue
+	 */
+	public function escapeAttribute( $text ) {
+		$text = $this->mStripState->unstripBoth( $text );
+		$text = Sanitizer::encodeAttribute( $text );
+		return $text;
+	}
+
+	/**
 	 * Accessor
 	 *
 	 * @return array

I did a fair amount of testing. Nothing broke that I saw. I think its unlikely this will break anything, as most of the html escaping in the parser is done via Sanitizer::removeHTMLtags, which already fixes strip markers in attributes

Anomie added a subscriber: Anomie.Oct 21 2015, 2:09 PM

Here's my patch for this (I can upload as an attachment if need be, I'm just a little worried I'll accidentally make the upload public by accident):

If you do it via https://phabricator.wikimedia.org/file/upload/, just be sure to change "Visible To" to "No One". When you then "transclude" it into this task then everyone who can see the task magically gets the ability to see the file too.

That patch doesn't cover OOUI, which we probably want to be covered too.

Patch for core:

Patch for ooui:

I wasn't sure what the best thing to do with 0x7F characters were (Its possibly illegal to have control characters in html5 docs anyways, so it felt kind of wrong to convert to &#127; The sanitizer at least thinks that &#127; is an illegal escape). I escaped it to 'SYMBOL FOR DELETE' (U+2421), which seemed fitting, but it might make an equal amount of sense to just remove them altogether.

Bawolff moved this task from Backlog to In Progress on the Security-Team board.
Bawolff added a project: Patch-For-Review.

Patch for core:

Core patch fixes the issue, and I think follows the security intent of Html::expandAttributes. It doesn't appear to break any parser or unit tests. The unknown is to what extent any extensions are abusing the current behavior. @tstarling, do you foresee any issues doing it this way?

Patch for ooui:

For the ooui patch, @Catrope, do you feel comfortable reviewing that? James suggested you or @matmarex.

The OOjs UI patch looks correct enough. That's the only place where we produce HTML output in PHP. I note that the strip marker escaping is only done to attributes, and not tag content, which as I understand it is intentional.

However… this feels like a really lame solution, and quite unreliable too. Does every MediaWiki developer now have to retrain themself that htmlspecialchars() is not sufficient to escape things for HTML, because you never know if they end up somewhere that strip markers are involved? Surely it's only a matter of time before we find another security hole.

However… this feels like a really lame solution, and quite unreliable too. Does every MediaWiki developer now have to retrain themself that htmlspecialchars() is not sufficient to escape things for HTML, because you never know if they end up somewhere that strip markers are involved? Surely it's only a matter of time before we find another security hole.

I agree, but I don't have a better solution.

This comment was removed by matmarex.

Thoughts:

  • We could make StripState aware that the string it was given to unstrip is HTML, parse it, and make sure that values of strip markers which are inside HTML attributes are HTML-escaped. (The problem is that the string is not always HTML, it's sometimes wikitext.)
    • Proof of concept: (very broken, all kinds of input gets all kinds of messed up; fixes Brian's examples, but not sure if it works for the general case)
  • We could change the strip markers so that htmlspecialchars() will mangle them (just put a '<' somewhere), and then change the Parser to use a custom method for escaping user input with strip markers, that would preserve the strip markers. (I think this is Sanitizer::removeHTMLtags?) This method would be exposed to be used by extensions whose authors thing they know what they're doing.
    • Proof of concept: (somewhat broken, headings and TOC seems to react poorly, but probably fixable with some effort; , fixes Brian's examples, might actually work for the general case)

That said, neither of these is a simple fix. Deploying Brian's patches to block the attacks he discovered sounds like a good idea.

matmarex edited the task description. (Show Details)Dec 1 2015, 9:51 PM

We could change the strip markers so that htmlspecialchars() will mangle them (just put a '<' somewhere), and then change the Parser to use a custom method for escaping user input with strip markers, that would preserve the strip markers. (I think this is Sanitizer::removeHTMLtags?) This method would be exposed to be used by extensions whose authors thing they know what they're doing.

I like that idea. Except lets use a double quote mark as that's less likely to mess things up then a < imo.

I'll try and play around making a patch doing that.

Using quote characters seemed to require minimal changes. Using both " and ' should force at least one to always need escaping in attributes.

I'm not sure what affect this would have on extensions. I think it would be rare for this to break an extension, but it is possible

In general I like the method, and I think we should go in this direction.

For pre and nowiki, I'm not sure why the original hooks were escaping ". Those seem to be the only callers of Xml::escapeTagsOnly in core, so it seems like that was intentional. @TimStarling last touched the code with c7a88753, but those tags using escapeTagsOnly predates that patch. I'm worried that side effect was relied on to sanitize the strings for inclusion on a quoted attribute somewhere.

In general I like the method, and I think we should go in this direction.

For pre and nowiki, I'm not sure why the original hooks were escaping ". Those seem to be the only callers of Xml::escapeTagsOnly in core, so it seems like that was intentional. @TimStarling last touched the code with c7a88753, but those tags using escapeTagsOnly predates that patch. I'm worried that side effect was relied on to sanitize the strings for inclusion on a quoted attribute somewhere.

Xml::escapeTagsOnly used to be known as wfEscapeHTMLTagsOnly() [2f12a58d93a] . It used to have a sister function wfEscapeHTML() [Basically corresponding to htmlspecialchars - 41c8b7bf7]. wfEscapeHTML() escaped ", <, >, and &, presumably for the same reason htmlspecialchars() does. I imagine wfEscapeHTMLTagsOnly() was modeled on it, just with the '&' removed, so that entities would not be double escaped. When <nowiki> needed something that escaped html but didn't double quote entities, I imagine that wfEscapeHTMLTagsOnly() seemed the obvious way to do it, and escaping the " was unnecessary but didn't cause any problems. Ultimately, wfEscapeHTMLTagsOnly() (And its usage in processing <nowiki> tags) dates back to the first revision of MW [d82c14f], so I imagine the true reasoning is lost to the sands of time.

I'm pretty sure that having unquoted double quotes output by <nowiki> should be safe.

Xml::escapeTagsOnly used to be known as wfEscapeHTMLTagsOnly() [2f12a58d93a] . It used to have a sister function wfEscapeHTML() [Basically corresponding to htmlspecialchars - 41c8b7bf7]. wfEscapeHTML() escaped ", <, >, and &, presumably for the same reason htmlspecialchars() does. I imagine wfEscapeHTMLTagsOnly() was modeled on it, just with the '&' removed, so that entities would not be double escaped. When <nowiki> needed something that escaped html but didn't double quote entities, I imagine that wfEscapeHTMLTagsOnly() seemed the obvious way to do it, and escaping the " was unnecessary but didn't cause any problems. Ultimately, wfEscapeHTMLTagsOnly() (And its usage in processing <nowiki> tags) dates back to the first revision of MW [d82c14f], so I imagine the true reasoning is lost to the sands of time.

I'm pretty sure that having unquoted double quotes output by <nowiki> should be safe.

That was the conclusion I came to as well, but I was hoping someone knew for sure. Maybe @tstarling remembers the history?

Otherwise, I think I'll deploy this as is on Tuesday to the wmf11 branch, so we get some time on the test wikis before it hits enwiki.

- updated bawolff's patch for short array syntax

Deployed this patch to wmf16 branch (currently test wikis). It will go to group 1 wikis today, so testing welcome.

17:53 csteipp: deployed patch for T110143 to wmf16

This patch is currently being discussed on English Wikipedia village pump, after the change in behavior created some issues when combined with Scribunto and Tidy: https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Whence_came_the_oddness_in_infobox_television_episode.3F (at the moment, it seems that nobody has guessed this is a security patch, but the change to behavior of strip markers is basically public now).

Hmm. So it looks almost like the css validation is takung place before strip markers are replaced. Which would probably be bad regardless. Although maybe the changes just meant that the strip marker wasnt being replaced, and then the css validator is barfing on that.

We should probably acknowladge the change was intentional even if we dont say why.

[I wonder if we should revert this until issue can be debugged. I probably wont have an oppurtunity to debug this until Monday]

Bawolff added a comment.EditedMar 14 2016, 3:51 AM

I'm investigating this a bit now. According to enwiki the problematic cases seem to be code like:

{{#tag:syntaxhighlight|<nowiki>d</nowiki>|lang="text"|enclose=none}}

(but not {{#tag:syntaxhighlight|<nowiki>hello</nowiki>}}. Several other variants of syntaxhighlight already don't work with nowiki prior to the patch on this bug)

The code seems to output an unchanged strip marker. An interesting affect of this is it doesn't show up on Special:Expandtemplates, due to the special:Expandtemplates double-parsing things bug. Also, tidy converts &#39; back to an apostrophe, which is kind of confusing too


The second issue identified, is basically T70011 all over again. mw.html lua library escapes quote character in attributes, breaking strip markers. This shows up differently in Special:Expandtemplates, due to double parsing, where the first (expand templates) run doesn't runt the css sanitizer, and the second round strips \x7F from its input.

I left a note on VP explaining that the issue is our fault, without going into details as to why. I didn't think it was fair to make the community play debugger well we know precisely what's going on. ( https://en.wikipedia.org/w/index.php?title=Wikipedia%3AVillage_pump_%28technical%29&type=revision&diff=709967437&oldid=709959675 )

I have patches for the issues enwiki is experiencing. I think that either they should be deployed asap, or the security patch should be temporarily reverted.

So the syntaxhighlight issue is that previously when the extension was in text mode, it didn't highlight any part of a strip marker, so by random chance, <nowiki> worked for some lexers (but not most)

This patch causes syntaxhighlight extension to expand strip markers before highlighting. This has been requested by people for quite a while, and makes a lot more logical sense than the previous behaviour. This fixes slightly more issues than this immediate bug, but it seems the most logical thing to do.

This makes mw.html lua library strip markers not mess with strip markers. It does this by unregexing entities, which is not the nicest, but best I could come up with. As an aside, I should note that lua actually had a parser test for this case, which the security patch broke.

As an aside, further examination seems to suggest that the syntaxhighlight thing was mostly limitted to a singletemplate was changed. Maybe we should concentrate on the lua change first. The lua change also seems very safe where the syntaxhighlight is changing how parameters are processed which while im pretty sure is fine and probably a really good idea, is something that ideally id want to be kind of public.

I was more comfortable with the Lua change as well. I'm planning to deploy that right after swat.

For the record, I think the lua change makes sense. I did some testing with lua when reviewing the initial patch, but missed that.

The syntax highlighting patch reminds me of another bug, but I'm having trouble finding it right now. I want to look into that more before we make too many changes. Maybe doing that one publicly would be a good idea.

The more i think about the syntax highlight one, the more im a bit worried about it.

It would also cause things like {{#tag:syntaxhighlight|{{special:recentchanges}}|lang=text|inline=1}} to output html as text instead of passing it through undistirbed (i think). Not sure if thats a bad thing or not.

This makes mw.html lua library strip markers not mess with strip markers. It does this by unregexing entities, which is not the nicest, but best I could come up with.

Code review: You should pass the actual prefixes in from PHP, instead of requiring that the Lua code be kept in sync whenever Parser::MARKER_PREFIX or Parser::MARKER_SUFFIX get changed again.

fixes that, and adds a PHPUnit test for it.

Even better. Thanks Brad.

16:39 csteipp: deployed fix for scribunto issue related to T110143

Deployed the fix to wmf16. There was a flurry of exceptions during the push-- probably a race between uniqPrefix being defined and used, and I should have synced those files in order. But looks like things are back to normal.

I gave a small update on VPT too.

@Anomie / @Bawolff, with that fix out, modules like Module:Convert doing things like remainder:match('^(\127[^\127]*UNIQ[^\127]*%-ref[^\127]*\127)(%s*)(.*)') should work ok, right? If so, I think it's just the syntax highlighting left. I'm looking into that also.

That particular pattern should be fine. If someone hard-coded the exact old prefix/suffix, those will need updating but there's nothing we can do about that unless we want to go fix it for them.

I think this may be the right fix. SyntaxHighlighter is running it's output through the equivalent of htmlspecialchars in this case, which if you syntax highlighted something that was put in an attribute, that would be the correct behavior.

So unstripping the input first would fix this particular issue, and I think would make the output of syntax highlighter more correct, since it wouldn't highlight and then expand something that was stripped, which would probably look odd.

@ori was the last one I know working on SyntaxHighlighter, but is there someone else who knows the extension well enough to make that call? Maybe @Reedy ?

csteipp raised the priority of this task from "High" to "Unbreak Now!".Mar 18 2016, 10:26 PM

Since we're a week in with the new patch, I think we're to the point where we either need to revert, or get fixes for the remaining issues.

Remaining issues:

  • Strip tags being shown for syntax highlighter
    • {{code|<nowiki>[[link]]s</nowiki>}}
      • (and {{bcode|anything}}, since that template uses passes the input to code in <nowiki> tags
    • {{#tag:syntaxhighlight|<nowiki>hello</nowiki>|lang="text"}}
  • Strip tags shown inside links

The syntax highlighting one we can probably fix-- bawolff's patch seems to me like the right direction. But having strip markers inside of links seems like we would need to change a lot of how link parsing is done in Parser to fix. And the math use case seems valid.

Should we just revert at this point?

  • Strip tags being shown for syntax highlighter
    • {{code|<nowiki>[[link]]s</nowiki>}}
      • (and {{bcode|anything}}, since that template uses passes the input to code in <nowiki> tags
    • {{#tag:syntaxhighlight|<nowiki>hello</nowiki>|lang="text"}}

I haven't reviewed any of the patches posted here, and I don't have any special relation to the extension, but I think it's reasonable for SyntaxHighlight to unstrip input before highlighting.

The ref thing is not new, I can reproduce on master too, this is filed as T27417. I don't have Math installed locally to check.

The ref thing is not new, I can reproduce on master too, this is filed as T27417. I don't have Math installed locally to check.

Thanks for the pointer! Indeed, <math> in link text produces strip markers on master too, so it's not due to the current patch. Possibly a recent change to Math.

Since it's friday evening, I'm not going to deploy the syntaxhighliter patch right now, but have testing it more, I think that's what we want.

<nowiki> inside of syntax highlighting for non-text languages was currently broken before the stripmarker patch. This would actually fix that use case.

I was debating if we should just unstrip nowiki instead of both, but if we do both, then we don't give back a strip marker if someone puts another parser function in the middle of the code... so I think unstripBoth is right.

I'll deploy Tuesday worst case.

Since it's friday evening, I'm not going to deploy the syntaxhighliter patch right now, but have testing it more, I think that's what we want.

<nowiki> inside of syntax highlighting for non-text languages was currently broken before the stripmarker patch. This would actually fix that use case.

I was debating if we should just unstrip nowiki instead of both, but if we do both, then we don't give back a strip marker if someone puts another parser function in the middle of the code... so I think unstripBoth is right.

Actually, I think we need to do only unstripNowiki(). If we unstrip General, that can cause a regression of T73167
by first doing frame:preprocess( " {{#tag:syntaxhighlight|{{special:Contributions/foo}}|lang=text}}" ) which returns a strip number like UNIQ--syntaxhighlight-0000001E-QINU (Slightly different in the new format). From there, replace syntaxhighlight with syntaxhighlightinner, and subtract 2 from the hex id number, and pass through unstripNoWiki: mw.text.unstripNoWiki( "UNIQ--syntaxhighlightinner-0000001C-QINU" ) . This reveals the html from the special page, allowing you to do the attack described in T73167

@Bawolff, you want to make a followup patch? I'm happy to make that one change too (I was testing with that earlier).

I'd like to get that deployed today if possible, just so this is wrapped up before the weekend.

Deployed the syntax highlighter patch, and things look better now,

22:10 csteipp: deployed followup patch for T110143
22:09 logmsgbot: csteipp@tin Synchronized php-1.27.0-wmf.18/extensions/SyntaxHighlight_GeSHi: (no message) (duration: 00m 29s)

It looks like the Scribunto patch got dropped from wmf18, I'm going to redeploy as soon as I'm on a stable connection.

Redeployed

to wmf18

03:45 csteipp: redeployed Scribunto patch for T110143

Bawolff closed this task as "Resolved".Mar 29 2016, 9:45 PM

Seems all done (Woo!)

If anyone encounters any other issues, please mention.

Another thing this might affect, is anything using strip markers in OOUI, as it uses htmlspecialchars for escaping body text. I don't think anything uses that that expects strip markers, but I do not know for sure.

To my knowledge the only potentially affected extension would be WikidataPageBanner, which can render a user-controlled OOjs UI icon, and it appears it doesn't handle strip markers at all. Try on Wikivoyage: {{PAGEBANNER:Example.jpg|icon-<nowiki>help</nowiki>=Main Page}} – this results in a strip marker in a class attribute. This is a pretty weird case though, as the banner is displayed outside of page content (similar to <indicator> tag) and I'm not sure if it would have worked more sensibly before the security patch anyway. I don't think there are currently any parser functions or extension tags that could produce OOjs UI widgets in page content (although there are some attempts pending and at least one abandoned, I suppose you might want to look at them on some lazy day :) ).

@Anomie / @Bawolff,

I backported the core patch to 1.25 as

, which was complicated because of I31d4556bbb07acb72c33fda335fa5a230379a03f, added in 1.26, which moved to a static prefix instead of a random one. The core part is pretty straightforward, but the Scribunto part then doesn't have a way to set the prefix in Scribunto_LuaHtmlLibrary.

It looks like the main (only?) use of the prefix when it's used to gsub the html-encoded version for the raw version in htmlEncode(). So it seems like setting the prefix to just "\x7f'\"`UNIQ" should be enough, since the Parser's mUniqPrefix will be "\x7f'\"`UNIQ" . self::getRandomString();, and getRandomString should just be hex characters. Or am I missing something there?

@Anomie / @Bawolff,

I backported the core patch to 1.25 as

, which was complicated because of I31d4556bbb07acb72c33fda335fa5a230379a03f, added in 1.26, which moved to a static prefix instead of a random one. The core part is pretty straightforward, but the Scribunto part then doesn't have a way to set the prefix in Scribunto_LuaHtmlLibrary.

It looks like the main (only?) use of the prefix when it's used to gsub the html-encoded version for the raw version in htmlEncode(). So it seems like setting the prefix to just "\x7f'\"`UNIQ" should be enough, since the Parser's mUniqPrefix will be "\x7f'\"`UNIQ" . self::getRandomString();, and getRandomString should just be hex characters. Or am I missing something there?

That sounds right to me

Sounds right to me too.

@Anomie, I was testing the scribunto backports and noticed that one of the unit tests is failing for all of the backports,

1) LuaStandalone: Scribunto_LuaCommonTests::testNoLeakedGlobals
The following globals are leaked: php
Failed asserting that 1 matches expected 0.

Is that from the changes to mw.html.lua? Is that something we need to fix?

@Bawolff, another oddity I'm seeing is that in 1.25/1.23, one of the parser tests fails after the patch (

/ ).

Running test 2. other tags... FAILED!
--- /tmp/mwParser-1184235810-expected	2016-04-26 16:16:31.381440158 -0700
+++ /tmp/mwParser-1184235810-actual	2016-04-26 16:16:31.381440158 -0700
@@ -1,3 +1,3 @@
 <p>&lt;div&gt;foo&lt;/div&gt;
-&lt;div style=&quot;color:red&quot;&gt;foo&lt;/div&gt;
+&lt;div style="color:red"&gt;foo&lt;/div&gt;
 </p>

The wikitext is,

<nowiki><div>foo</div>
<div style="color:red">foo</div></nowiki>

so I'm suspicious that allowing " through in CoreTagHooks::nowiki is the issue, but I'm not sure why the test is passing for 1.26 in that case. Any ideas?

@Anomie, I was testing the scribunto backports and noticed that one of the unit tests is failing for all of the backports,

1) LuaStandalone: Scribunto_LuaCommonTests::testNoLeakedGlobals
The following globals are leaked: php
Failed asserting that 1 matches expected 0.

Is that from the changes to mw.html.lua? Is that something we need to fix?

Good catch. Yeah, that needs fixing in the master patch too (I must have forgotten to run the full test suite). Fortunately the fix is simple enough, just delete this line that got added to engines/LuaCommon/lualib/mw.html.lua, it's not needed there:

php = mw_interface

I can post a fresh version of the master patch if you want me to do it, just let me know.

I can post a fresh version of the master patch if you want me to do it, just let me know.

I can update all the patches I have locally. Thanks!

New patches:

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:27 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.
Restricted Application added subscribers: TerraCodes, Urbanecm, Malyacko. · View Herald TranscriptMay 20 2016, 5:27 PM

Change 289884 merged by jenkins-bot:
SECURITY: Replace strip markers before syntax highlighting

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

Change 289885 merged by jenkins-bot:
SECURITY: Replace strip markers before syntax highlighting

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

Change 290157 had a related patch set uploaded (by Brian Wolff):
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290158 had a related patch set uploaded (by Brian Wolff):
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290159 had a related patch set uploaded (by Brian Wolff):
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290160 had a related patch set uploaded (by Brian Wolff):
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290157 merged by jenkins-bot:
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290159 merged by jenkins-bot:
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290158 merged by Brian Wolff:
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Change 290160 merged by Brian Wolff:
SECURITY: Don't escape strip markers when escaping attributes in mw.html

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

Restricted Application added a subscriber: Jay8g. · View Herald TranscriptJan 14 2017, 2:40 AM