MediaWiki r66990 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r66989‎ | r66990 (on ViewVC)‎ | r66991 >
Date:05:02, 28 May 2010
Author:tstarling
Status:ok (Comments)
Tags:
Comment:
Normalise CSS escape sequences.
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r66992MFT r66990 (CSS escape sequence normalisation), and updates for release of 1....tstarling07:08, 28 May 2010
r66993MFT r66990 (CSS escape sequence normalisation), and updates for release of 1....tstarling07:09, 28 May 2010
r66994MFT r66990: CSS escape sequence normalisationtstarling07:10, 28 May 2010
r67101Follow up r66990. Fix parser tests.platonides18:59, 30 May 2010
r67630MFT r67101 fixing in branch parser tests broken in r66990....platonides16:06, 8 June 2010

Comments

#Comment by OverlordQ (talk | contribs)   17:38, 30 May 2010

Parser tests need updated.

9 previously passing test(s) now FAILING! :(

  • <pre> with forbidden attribute values (bug 3202) [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • Bug 2304: HTML attribute safety (dangerous style template; 2309) [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • Bug 2304: HTML attribute safety (unsafe parameter; 2309) [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • Bug 3244: HTML attribute safety (extension; unsafe) [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • MSIE CSS safety test: spurious slash [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • MSIE CSS safety test: hex code [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • Table attribute safety [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • CSS line continuation 1 [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
  • CSS line continuation 2 [Introduced between 30-May-2010 17:30:20, 1.17alpha (r66989) and 30-May-2010 17:30:56, 1.17alpha (r66990)]
#Comment by CMBJ (talk | contribs)   21:14, 1 June 2010

r66990 appears to have made transparency [filter: alpha(opacity=70);] impossible on some browsers.

#Comment by Tim Starling (talk | contribs)   00:13, 2 June 2010

The filter property is insecure and can lead to complete compromise of the client computer, see http://seclists.org/bugtraq/2010/May/228

#Comment by CMBJ (talk | contribs)   09:43, 2 June 2010

Could we not specifically restrict 'ICMFilter' instead?

#Comment by Tim Starling (talk | contribs)   12:59, 2 June 2010

No. AlphaImageLoader is also a security vulnerability, it allows users to load arbitrary URLs without requiring the blacklisted url() markup. The potential for similar security vulnerabilities is unlimited. IE extensions may define their own filter objects, MSDN provides complete documentation and tutorials explaining how to do this. And Microsoft may add new filters in future versions of IE which open up new security vulnerabilities.

We would have to parse it and whitelist certain "known-good" filters instead, which would be challenging since the format of the filter property appears to be undocumented, and is subject to change. I'd rather spend my time working on better standards-compliance.

CSS 3 has an opacity property which does what you want to do, you should use that instead. It works on all browsers except one.

#Comment by Tim Starling (talk | contribs)   06:12, 11 June 2010

Marking new since the parser tests were fixed in r67101.

#Comment by Hashar (talk | contribs)   18:25, 15 November 2010

There was no activity on this CR revision and it got merge to trunk. Could we assume it as "ok" ?

#Comment by Happy-melon (talk | contribs)   15:32, 15 December 2010

It's been running on WMF for six months; if there were a regression obvious enough to spot in CR, someone would have seen it by now.

Status & tagging log