mick wrote:

Four patched extension files

attachment DocumentApproval.tgz ignored as obsolete

Could(In reply to comment #1)

Created an attachment (id=4981) [details]
Four patched extension files

Could you please attach a non-TGZed patch?

mick wrote:

Same patch in zip format

attachment DocumentApproval.zip ignored as obsolete

(In reply to comment #3)

Created an attachment (id=4982) [details]
Same patch in zip format

I meant plain text :D sorry

mick wrote:

DocumentApproval.body.php

Attached:

mick wrote:

DocumentApproval.i18n.php

attachment DocumentApproval.i18n.php ignored as obsolete

mick wrote:

DocumentApproval.i18n.php

Attached:

mick wrote:

DocumentApproval.php

Attached:

mick wrote:

DocumentApproval.sql

Attached:

Why is a bugzilla entry being added for this extension?

It's not maintained in SVN, it's a wiki page. If the code needs to be edited, just edit the page there.

If it were in SVN already, you would have only needed a single diff file. And if this was supposed to be committed to SVN, then it would have been best to commit it, then hand out a diff file to patch that.

Overall that extension has not improved much. The message loading is non-standard, it's using ugly raw SQL when it should be using database methods. It shouldn't be declaring two extension contribs items for itself. And that .sql file, it shouldn't be including a DROP in there, that's just plain dangerous and is likely going to cause people to accidentally delete all their stored data for the extension. Not to mention it's not using the variables to make it compatible with wiki using prefixes (ie: It can't be nicely used with patchSql.php which makes it even more dangerous). It's not autoloading classes. The .sql file specifies char-sets overriding what the user may want, and does not respect the type= settings configured for the wiki. The parser functions are also being loaded in the wrong way. It even has hardcoded HTML which will break any notion of XHTML validity, and it's even using hardcoded colors using an attribute depreciated even in plain old HTML. It's also writing to a slave database pretending it's the master (So once you put it on any site that actually want's to scale, the data will go completely corrupt). It's even using the wrong method for grabbing the current user, one which may (depending on how it's coded) end up duplicating the same session check twice on a page load. It's also using a list in an unreliable way, there is no limit, so it's liable that extra data may end up trimmed off if someone decides "Hey, Let's include my Middle name in my Real name" or has a name which requires a space in it (Yes, there are names like that, I even know a person with a two word middle name). It even uses database creation statements, inside of the extension, where the web DB user isn't supposed to have the privileges to do stuff like that. It's also using a unnecessary bit of raw sql to grab the user's real name, when the User object has a method to get it for you. On top of it loading extension messages into the system when they're not used, it loads them twice when they actually are used.

It's also quite scary that mere skin tabs are calling SQL queries to the database. That kind of thing deserves caching support at the least.

Oh, by the way... there is a small chance of someone using a username passed to one of the functions in the extension to create a SQL injection attack. Oh ya, and it's also got XSS vulnerabilities through MW messages. Oh yes, I think I also see another SQL injection vector in fnDocumentApproval. Perhaps even easier to exploit than the first, and they're connected to... You could potentially inject 2 SQL statements on the same line.

Just an external note... But the license statement does not include "version 2 or later"... in other words the license is pretty incompatible if some day in the distant future MediaWiki and many of it's extensions decide to go GPLv3.

Ok, that's probably enough for my comment.

INVALID. Not maintained through this bug tracker. Our apologies for any possible confusion.

tim.starling wrote:

content hidden as private in Bugzilla