Last modified: 2011-11-22 04:10:36 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 14555 - Codefix for extension DocumentApproval
Codefix for extension DocumentApproval
Status: RESOLVED INVALID
Product: MediaWiki extensions
Classification: Unclassified
General/Unknown (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
http://www.mediawiki.org/wiki/Extensi...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-16 12:08 UTC by Mick van der Most van Spijk
Modified: 2011-11-22 04:10 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Four patched extension files (5.10 KB, patch)
2008-06-16 12:09 UTC, Mick van der Most van Spijk
Details
Same patch in zip format (6.22 KB, patch)
2008-06-16 12:19 UTC, Mick van der Most van Spijk
Details
DocumentApproval.body.php (16.48 KB, patch)
2008-06-16 13:15 UTC, Mick van der Most van Spijk
Details
DocumentApproval.i18n.php (1.55 KB, application/octet-stream)
2008-06-16 13:15 UTC, Mick van der Most van Spijk
Details
DocumentApproval.i18n.php (1.55 KB, patch)
2008-06-16 13:15 UTC, Mick van der Most van Spijk
Details
DocumentApproval.php (2.19 KB, patch)
2008-06-16 13:15 UTC, Mick van der Most van Spijk
Details
DocumentApproval.sql (1.95 KB, patch)
2008-06-16 13:16 UTC, Mick van der Most van Spijk
Details

Description Mick van der Most van Spijk 2008-06-16 12:08:19 UTC
Fixed:
- undeclared variables
- code style
- parsing of names and roles
- added dutch language
- added check on anonymous user
Comment 1 Mick van der Most van Spijk 2008-06-16 12:09:09 UTC
Created attachment 4981 [details]
Four patched extension files
Comment 2 Roan Kattouw 2008-06-16 12:10:30 UTC
Could(In reply to comment #1)
> Created an attachment (id=4981) [details]
> Four patched extension files
> 

Could you please attach a non-TGZed patch?
Comment 3 Mick van der Most van Spijk 2008-06-16 12:19:04 UTC
Created attachment 4982 [details]
Same patch in zip format
Comment 4 Roan Kattouw 2008-06-16 12:19:58 UTC
(In reply to comment #3)
> Created an attachment (id=4982) [details]
> Same patch in zip format
> 

I meant plain text :D sorry
Comment 5 Mick van der Most van Spijk 2008-06-16 13:15:10 UTC
Created attachment 4983 [details]
DocumentApproval.body.php
Comment 6 Mick van der Most van Spijk 2008-06-16 13:15:29 UTC
Created attachment 4984 [details]
DocumentApproval.i18n.php
Comment 7 Mick van der Most van Spijk 2008-06-16 13:15:32 UTC
Created attachment 4985 [details]
DocumentApproval.i18n.php
Comment 8 Mick van der Most van Spijk 2008-06-16 13:15:50 UTC
Created attachment 4986 [details]
DocumentApproval.php
Comment 9 Mick van der Most van Spijk 2008-06-16 13:16:42 UTC
Created attachment 4987 [details]
DocumentApproval.sql
Comment 10 Daniel Friesen 2008-06-16 14:21:08 UTC
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.
Comment 11 Siebrand Mazeland 2008-08-11 20:01:47 UTC
INVALID. Not maintained through this bug tracker. Our apologies for any possible confusion.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links