Last modified: 2011-10-20 02:42:26 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 31723 - CheckUser API module
CheckUser API module
Product: MediaWiki extensions
Classification: Unclassified
CheckUser (Other open bugs)
All All
: Unprioritized enhancement (vote)
: ---
Assigned To: cryptocoryne
: patch, patch-need-review
: 15129 (view as bug list)
Depends on:
Blocks: noncoreapi
  Show dependency treegraph
Reported: 2011-10-15 09:30 UTC by cryptocoryne
Modified: 2011-10-20 02:42 UTC (History)
8 users (show)

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

patch (19.53 KB, patch)
2011-10-15 09:30 UTC, cryptocoryne
new version of patch (16.81 KB, patch)
2011-10-16 23:37 UTC, cryptocoryne
patch (17.65 KB, patch)
2011-10-17 07:31 UTC, cryptocoryne

Description cryptocoryne 2011-10-15 09:30:48 UTC
Created attachment 9237 [details]

API module for CheckUser extension.
Comment 1 Bryan Tong Minh 2011-10-15 09:47:42 UTC
Thank you for submitting a patch.

Unfortunately, this patch has the same problems as the one on bug 15129, as I noted in comment 26:

> Especially with a high sensitivity module such as CU, I really do want to have
> a common backend for the special page and the API module. With all the code
> duplication the chances that are high that security vulnerabilities and other
> bugs that are found in the API module and fixed are not propagated to the
> special page and vice versa. Considering the sensitivity of CheckUser I do not
> think that this is acceptable.

*** This bug has been marked as a duplicate of bug 15129 ***
Comment 2 cryptocoryne 2011-10-15 09:57:28 UTC
Hm, I don't duplicate existing CheckUser functions. Only two functions taken from main CheckUser class.

You suggest rewrite xff as boolean parameter? Or something changes?

This patch tested on trunk version of MediaWiki and don't have vulnerabilities - for validations IPs and usernames used same functions as on main CheckUser class (call them from this class is not real, but this functions marked as protected).
Comment 3 Bryan Tong Minh 2011-10-15 10:12:14 UTC
(In reply to comment #2)
> Hm, I don't duplicate existing CheckUser functions. Only two functions taken
> from main CheckUser class.
With code duplication I mean copy-pasting of functions. My point is that by doing all the backend work in different places for the UI and API, it is easy for those code paths to diverge, which is an increased in maintenance burden, and is a potential risk for future security problems. 
There are numerous essays on the web that do a better job in explaining why copy-pasting code is bad than me. See for example [1], under the heading "Why is code duplication bad?"

Comment 4 cryptocoryne 2011-10-15 10:17:36 UTC
Ok, thanks for explanation.

I rewrite this functions and xff requests syntax during the one-two days. However, rewrite addLogEntry function is difficult but performed by this function actions is absolute identical for specialpage and API.

You can see ways to solve this problem? Maybe, need declare addLogEntry function in CheckUser class as public (I don't see any vulnerabilities with this function) or some other way?
Comment 5 Bryan Tong Minh 2011-10-15 20:57:03 UTC
Yeah a whole lot of these protected functions in SpecialCheckUser should be public static, or even moved to a separate class.

Ideally, you would be able to do something like

$users = CheckUser::doIPUsersRequest()

This is quite a lot of work, with all the $wgOut calls intermixed with the database querying.
Comment 6 cryptocoryne 2011-10-16 23:37:59 UTC
Created attachment 9243 [details]
new version of patch

I rewrite code of API module, remove copy-pasted code and change used classes of CheckUser to public static and their calls.
Comment 7 Bryan Tong Minh 2011-10-17 07:14:16 UTC
Looks good. Can you make the diff in unified diff format (specify -u on the command line I believe)
Comment 8 cryptocoryne 2011-10-17 07:31:01 UTC
Created attachment 9245 [details]

Comment 9 Sumana Harihareswara 2011-10-17 19:25:04 UTC
Thank you for the patch, cryptocoryne!  Mark and I will try to get someone to review it sooner rather than later.  :-)
Comment 10 John Du Hart 2011-10-18 23:30:31 UTC
Very well done, cryptocoryne. That patch was near flawless and was committed in r100165.  Just a few notes on stuff I changed so you can learn:

In ApiQueryCheckUser you I had this line:

  +        $reason = 'API: ' . $params['reason'];

Which I changed to:

  +        $reason = wfMsgForContent( 'checkuser-reason-api' ) . ' ' . $params['reason'];

This allows the 'API:' part to be translated.
Comment 11 John Du Hart 2011-10-19 09:46:55 UTC
*** Bug 15129 has been marked as a duplicate of this bug. ***
Comment 12 Platonides 2011-10-19 16:15:08 UTC
Well, it had calls like $this->addWhere( "cuc_user_text = '$target'" ); which allowed SQL injection...

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