Last modified: 2008-06-27 19:51:02 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 13426 - Admins should be asked for confirmation when Antispoof applies
Admins should be asked for confirmation when Antispoof applies
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
AntiSpoof (Other open bugs)
unspecified
All All
: Normal enhancement with 8 votes (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch
: 13917 (view as bug list)
Depends on:
Blocks: 14576
  Show dependency treegraph
 
Reported: 2008-03-18 22:01 UTC by gdonato
Modified: 2008-06-27 19:51 UTC (History)
6 users (show)

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


Attachments
trunk/extensions/AntiSpoof/AntiSpoof.php - Adds warning to sysops before creating similar username. (723 bytes, patch)
2008-06-19 19:19 UTC, Tyler Romeo
Details
Incorporates HTML form in code instead of in message. (1.79 KB, patch)
2008-06-20 00:38 UTC, Tyler Romeo
Details
Adds option for extra input. See other diffs for better description. - Capitalization fixed. (1.27 KB, patch)
2008-06-21 03:11 UTC, Tyler Romeo
Details
Adds option for extra input in mainloginform function. See other diffs for better description. (1.74 KB, patch)
2008-06-21 03:12 UTC, Tyler Romeo
Details
Adds support for an extra <input> tag in the login form. - Fixed capitalization and used htmlspeciachars() directly. (807 bytes, patch)
2008-06-21 03:18 UTC, Tyler Romeo
Details
Adds option for extra input in mainloginform function. See other diffs for better description. (Now checks for array) - Fixed capitalization. (1.84 KB, patch)
2008-06-21 03:44 UTC, Tyler Romeo
Details
Combined patch. (3.94 KB, patch)
2008-06-27 14:13 UTC, Tyler Romeo
Details

Description gdonato 2008-03-18 22:01:08 UTC
When an administrator creates an account which would normally be blocked by the Antispoof extension, they should be asked for confirmation that they really want to create the account before it is actually created (the confirmation should include which username(s) the account is similar to) Note: Multiple results wanted in bug 12232.

Rationale: Without this, administrators can accidentally create accounts (on behalf of someone else) which are confusing as they are similar to another user. At worst, this would mean one extra click if the admin already knew about the conflict but would prevent the admin accidentally creating a confusing situation.
Comment 1 gdonato 2008-05-01 16:38:40 UTC
*** Bug 13917 has been marked as a duplicate of this bug. ***
Comment 2 Tyler Romeo 2008-06-19 19:19:16 UTC
Created attachment 5002 [details]
trunk/extensions/AntiSpoof/AntiSpoof.php - Adds warning to sysops before creating similar username.

Patch against trunk.

The patch makes it where if a sysop attempts to create a similar username, the creation fails with a message at the top of the login form. This message includes a built-in form that goes through with the creation. The message is "antispoof-name-warning", and a draft of the message can be found at [[User:Parent5446/MediaWiki/antispoof-name-warning]]. The warning is surpressed by POSTing "wpOverride" to the login form. Note: Users still must have the "override-antispoof" right in order to override.
Comment 3 Ilmari Karonen 2008-06-19 21:50:20 UTC
I don't think we want to embed form HTML in messages like that, even if it technically works.  Better to construct the form in code and just pull the strings from messages.
Comment 4 Tyler Romeo 2008-06-20 00:38:45 UTC
Created attachment 5004 [details]
Incorporates HTML form in code instead of in message.

OK, now the form is located in the code, and only the beginning text is in the message.
Comment 5 Brion Vibber 2008-06-20 18:29:57 UTC
This patch has some basic security problems: arbitrary user input is placed directly into HTML output without escaping, making it an HTML injection / cross-site scripting vulnerability.

It also appears to rely on client-side JavaScript to submit a form via clicking a link instead of using a regular form button. This practice should be avoided, as it doesn't follow standard user interface behavior and it will fail if JavaScript is disabled.

The action path in the form is hardcoded, and will fail on most MediaWiki sites including Wikimedia's on the secure.wikimedia.org interface.

Comment 6 Tyler Romeo 2008-06-20 19:58:28 UTC
OK, sorry about that. I am not exactly the most experienced PHP user. I'll see what I can do and get back to this. It may require a bit more tweaking than usual, but that's OK.
Comment 7 Tyler Romeo 2008-06-21 03:11:17 UTC
Created attachment 5006 [details]
Adds option for extra input. See other diffs for better description. - Capitalization fixed.

Adds extrainput array when running hook. See later diffs for more info.
Comment 8 Tyler Romeo 2008-06-21 03:12:47 UTC
Created attachment 5007 [details]
Adds option for extra input in mainloginform function. See other diffs for better description.

This adds the option for the $extrainput array, which is passed from the AntiSpoof extension (or any other extension that wants to use it) to the next diff, which is in the Userlogin.php file.
Comment 9 Tyler Romeo 2008-06-21 03:18:14 UTC
Created attachment 5008 [details]
Adds support for an extra <input> tag in the login form. - Fixed capitalization and used htmlspeciachars() directly.

This adds support for another variable in the account creation form. If a function passes an array $extrainput, with a 'name' and 'value', the account creation form will have a hidden variable with the following syntax:

<input type="hidden" name="$extrainput['name']" value="$extrainput['value']" />

In the AntiSpoof extension specifically, here is a conclusion of what happens. If the extension sets the conflict mode to 'OVERRIDE' (which it does for users with the 'override-antispoof' right), then the function will return false, and cause the Special:Userlogin page (along with classes in the Userlogin.php file) to generate the account creation form with a notice on top. The concept is basically like when the software warns a user about not putting in an edit summary. If the username is similar, it pops up with a notice saying "The accounts are similar, press Create account (or by-email) again to continue." When they press it a second time, the form now POSTs the wpOverride variable, which was inserted using the $extrainput array, which allows the creation to go through. Look at the code itself for more details, since my explanation is a little confusing.
Comment 10 Tyler Romeo 2008-06-21 03:44:39 UTC
Created attachment 5009 [details]
Adds option for extra input in mainloginform function. See other diffs for better description. (Now checks for array) - Fixed capitalization.
Comment 12 Ilmari Karonen 2008-06-25 20:18:37 UTC
Looks pretty good, though you should be using htmlspecialchars() directly instead of $this->msg() in templates/Userlogin.php.  Also maybe make it a checkbox instead, and add a custom label (for which you _should_ use $this->msg())?  Also, in the future, I'd suggest submitting all the relevant changes in one patch unless some parts are genuinely alternative or optional -- it's less confusing for the devs, who don't need to puzzle out which patches you want them to apply.
Comment 13 Bryan Tong Minh 2008-06-25 21:58:01 UTC
Please watch your case sensitivity on $extraInput, keep variable names with the same capitalization everywhere you use them. 
Comment 14 Tyler Romeo 2008-06-26 00:56:33 UTC
Comment on attachment 5006 [details]
Adds option for extra input. See other diffs for better description. - Capitalization fixed.

>Index: extensions/AntiSpoof/AntiSpoof.php
>===================================================================
>--- extensions/AntiSpoof/AntiSpoof.php	(revision 36517)
>+++ extensions/AntiSpoof/AntiSpoof.php	(working copy)
>@@ -60,7 +60,7 @@
>  * @param string $message
>  * @return bool true to continue, false to abort user creation
>  */
>-function asAbortNewAccountHook( $user, &$message ) {
>+function asAbortNewAccountHook( $user, &$message, &$extraInput ) {
> 	global $wgAntiSpoofAccounts, $wgUser;
> 	wfLoadExtensionMessages( 'AntiSpoof' );
> 
>@@ -84,6 +84,7 @@
> 			wfDebugLog( 'antispoof', "{$mode}PASS new account '$name' [$normalized]" );
> 		} else {
> 			wfDebugLog( 'antispoof', "{$mode}CONFLICT new account '$name' [$normalized] spoofs '$conflict'" );
>+			global $wgRequest;
> 			if( $active ) {
> 				$message = wfMsg( 'antispoof-name-conflict', $name, $conflict );
> 				return false;
>@@ -88,6 +89,11 @@
> 				$message = wfMsg( 'antispoof-name-conflict', $name, $conflict );
> 				return false;
> 			}
>+			elseif( $mode = 'OVERRIDE' && !$wgRequest->getText( 'wpOverride' ) ) {
>+				$message = wfMsg( 'antispoof-name-warning', $name, $conflict );
>+				$extrainput = array( 'name' => 'wpOverride', 'value' => true );
>+				return false;
>+			}
> 		}
> 	} else {
> 		$error = $spoof->getError();
Comment 15 Tyler Romeo 2008-06-26 00:59:29 UTC
Comment on attachment 5008 [details]
Adds support for an extra <input> tag in the login form. - Fixed capitalization and used htmlspeciachars() directly.

>Index: phase3/includes/templates/Userlogin.php
>===================================================================
>--- phase3/includes/templates/Userlogin.php	(revision 36517)
>+++ phase3/includes/templates/Userlogin.php	(working copy)
>@@ -208,6 +208,11 @@
> 					tabindex="9"
> 					value="<?php $this->msg('createaccountmail') ?>" />
> 				<?php } ?>
>+				<?php if( is_array( $this->data['extraInput'] ) && $this->data['extraInput']['name'] ) { ?>
>+				<input type='hidden' name="<?php $this->htmlspecialchars( $this->data['extraInput']['name'] ); ?>" id="<?php $this->msg( $this->data['extraInput']['name'] ); ?>"
>+					value="<?php if( $this->data['extraInput']['value'] ) {  $this->msg( $this->data['extraInput']['value'] ); }
>+						     else { echo 0; } ?>" />
>+				<?php } ?>
> 			</td>
> 		</tr>
> 	</table>
Comment 16 Tyler Romeo 2008-06-26 01:00:31 UTC
Comment on attachment 5009 [details]
Adds option for extra input in mainloginform function. See other diffs for better description. (Now checks for array) - Fixed capitalization.

>Index: phase3/includes/specials/SpecialUserlogin.php
>===================================================================
>--- phase3/includes/specials/SpecialUserlogin.php	(revision 36517)
>+++ phase3/includes/specials/SpecialUserlogin.php	(working copy)
>@@ -297,10 +297,14 @@
> 		$u->setRealName( $this->mRealName );
> 
> 		$abortError = '';
>-		if( !wfRunHooks( 'AbortNewAccount', array( $u, &$abortError ) ) ) {
>+		if( !wfRunHooks( 'AbortNewAccount', array( $u, &$abortError, &$extraInput ) ) ) {
> 			// Hook point to add extra creation throttles and blocks
> 			wfDebug( "LoginForm::addNewAccountInternal: a hook blocked creation\n" );
>-			$this->mainLoginForm( $abortError );
>+			if( is_array( $extraInput ) ) {
>+				$this->mainLoginForm( $abortError, $extraInput );
>+			} else {
>+				$this->mainLoginForm( $abortError );
>+			}
> 			return false;
> 		}
> 
>@@ -710,7 +714,7 @@
> 	/**
> 	 * @private
> 	 */
>-	function mainLoginForm( $msg, $msgtype = 'error' ) {
>+	function mainLoginForm( $msg, $msgtype = 'error', $extraInput ) {
> 		global $wgUser, $wgOut, $wgAllowRealName, $wgEnableEmail;
> 		global $wgCookiePrefix, $wgAuth, $wgLoginLanguageSelector;
> 		global $wgAuth, $wgEmailConfirmToEdit;
>@@ -793,6 +797,15 @@
> 		$template->set( 'canreset', $wgAuth->allowPasswordChange() );
> 		$template->set( 'remember', $wgUser->getOption( 'rememberpassword' ) or $this->mRemember  );
> 
>+		if( is_array( $extraInput ) && isset( $extraInput['name'] ) ) {
>+			if( isset( $extraInput['value'] ) ) {
>+				$template->set( 'extraInput', array( 'name' => $extraInput['name'], 'value' => $extraInput['value'] ) );
>+			}
>+			else {
>+				$template->set( 'extraInput', array( 'name' => $extraInput['name'], 'value' => false ) );
>+			}
>+		}
>+
> 		# Prepare language selection links as needed
> 		if( $wgLoginLanguageSelector ) {
> 			$template->set( 'languages', $this->makeLanguageSelector() );
Comment 17 Tyler Romeo 2008-06-26 01:01:53 UTC
I fixed capitalization in all the patches so they all have the $extraInput version. Furthermore, it now uses htmlspecialchars() directly instead of the msg() function.
Comment 18 Tyler Romeo 2008-06-27 14:13:08 UTC
Created attachment 5025 [details]
Combined patch.

I combined the patches into one diff for easier viewing.
Comment 19 Bryan Tong Minh 2008-06-27 19:51:02 UTC
Implemented a more flexible way of modifying the user login form in r36758 based on Tyler Romeo's patch.

Only override the anti spoof if the wpIgnoreAntiSpoof checkbox is set in r36760.

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


Navigation
Links