Page MenuHomePhabricator

Update ContactPage for IP masking
Closed, ResolvedPublic

Description

From @Tgr :

ContactPage has some logic to format messages from logged-in users and anons differently (e.g. username vs. IP). It should probably just treat temp users as logged-in.

Content-Platform-Team doesn't have a lot of experience with ContactPage, but from that spec we ought to be able to do this update.

Event Timeline

I debug Contact extension locally, and noticed that user's IP address is added to the email message title.
Here is the logic for adding IP address (link):

$includeIP = isset( $config['IncludeIP'] ) && $config['IncludeIP']
&& ( $user->isAnon() || $formData['IncludeIP'] );

Depending on temp user account type (whether it should be isAnon or has some specific role, see this discussion) the implementation of the code block above should be changed.

In general, IP addresses should still be sent (or at least forms should be configurable to allow IP addresses to be sent). This information is necessary for the Stewards IP block appeal and exemption workflow.

If I understand this correctly, the code as is appropriately reports the IP address in a contact page email and is/maybe needed to aid in an IP block appeal exemption, and as such removing the IP here is not correct nor appropriate as part of the ongoing IPMasking issue. Let me know if I should close this ticket, or let others comment/review.

In general, IP addresses should still be sent (or at least forms should be configurable to allow IP addresses to be sent). This information is necessary for the Stewards IP block appeal and exemption workflow.

		if ( $user->isRegistered() ) {
			// Use real name if set
			$realName = $user->getRealName();
			if ( $realName ) {
				$fromName = $realName;
			} else {
				$fromName = $user->getName();
			}
			$fromAddress = $user->getEmail();
		}

this should probably use !$user->isAnon().

		$includeIP = isset( $config['IncludeIP'] ) && $config['IncludeIP']
			&& ( $user->isAnon() || $formData['IncludeIP'] );

this is probably fine as it is, we don't want to send temp users' IPs by default.

I can apply this change, but without commenting out lines 132-135 in SpecialContact.php like this, I cannot ever reach line 164, so unless I am missing something, this code is currently unreachable, or I cannot create a test environment condition with a logged out browser to reach it as a temporary user, which also maybe a configuration I have not set correctly in my local wiki instance:

[line 131]		$recipient = User::newFromName( $config['RecipientUser'] );
//		if ( $recipient === null || !$recipient->canReceiveEmail() ) {
//			$this->getOutput()->showErrorPage( 'noemailtitle', 'noemailtext' );
//			return;
//		}
In T335962#8897361, @Tgr wrote (and Sbailey merged the change recommended):
[line 164]         if ( !$user->isAnon() ) { // Was if ( $user->isRegistered() )
			// Use real name if set
			$realName = $user->getRealName();
			if ( $realName ) {
				$fromName = $realName;
			} else {
				$fromName = $user->getName();
			}
			$fromAddress = $user->getEmail();
		}

I cannot ever reach line 164, so unless I am missing something

$recipient is user that's going to receive the email, as set in $wgContactConfig['default']['RecipientUser']. That needs to be a valid account on the wiki, with a verified email. You can set $wgEmailAuthentication = false; for the purpose of testing.

$user, on the other hand, is the sender, which can be logged out.

Change 926585 had a related patch set uploaded (by Sbailey; author: Sbailey):

[mediawiki/extensions/ContactPage@master] Do not display users IP as User Name in ContactPage for anonymous users

https://gerrit.wikimedia.org/r/926585

Thanks Arlo, also had to create a recipient account and fix the recipientUser string to match and then things worked.

The current code using isRegistered() but should be using isAnon() as Tgr pointed out and the patch I submitted changes that. I tested this as best I can with a local wiki and the email in this case does contain the the email header:
'Contact message testing ... (from Shannon at 127.0.0.1)'

I assume that on a production wiki, the users IP address included would not be localhost.

Dreamy_Jazz subscribed.
		if ( $user->isRegistered() ) {
			// Use real name if set
			$realName = $user->getRealName();
			if ( $realName ) {
				$fromName = $realName;
			} else {
				$fromName = $user->getName();
			}
			$fromAddress = $user->getEmail();
		}

this should probably use !$user->isAnon().

I'm not so sure about that, as temporary users cannot have preferences which means that they cannot have an email address and/or real name. Checking for whether the user is named makes more sense in this case to me.

I'm not so sure about that, as temporary users cannot have preferences which means that they cannot have an email address and/or name. Checking for whether the user is named makes more sense in this case to me.

But they do have a username, and that is how you want to identify them to the person reading the email.

I'm not so sure about that, as temporary users cannot have preferences which means that they cannot have an email address and/or name. Checking for whether the user is named makes more sense in this case to me.

But they do have a username, and that is how you want to identify them to the person reading the email.

Okay. I missed that the code also checks for the name (and not the real name).

Change 926585 abandoned by Sbailey:

[mediawiki/extensions/ContactPage@master] Corrects sending users IP in ContactPage for anonymous users

Reason:

https://gerrit.wikimedia.org/r/926585

MSantos triaged this task as High priority.Aug 15 2024, 2:11 PM
MSantos moved this task from Blocked to Current Deploy Target on the Content-Transform-Team-WIP board.

I looked at the discussion here, the subtasks, and the ContactPage code, consulted some folks, and here is my understanding.

  • @Dreamy_Jazz replaced use of isRegistered with isNamed as part of T344722 to ensure real names are only used for non-temp registered users (as is the intention).
  • Previously, anon users of the contact form would have their IP sent over unconditionally. But, registered users would get a checkbox to choose if their IP is sent. With temp users, the expectation is that we are going to also present them with a checkbox to give them the option to hide their IP. If that is true, then there is no other change necessary to the code and this task is resolved.
  • But, if the product expectation is that temp users using ContactPage don't get to hide their IPs, the code need to be tweaked slightly -- easy to make the change.

I think this form configuration is consistent with ACN's request in T335962#8897275. If this becomes unworkable for stewards and/or the product requirements change here, we can very easily change this behavior for temp users.

So, working off the assumption that the product requirement here is that Temp Users can choose to hide their IPs, I'm going to leave this ticket open for the next week and close this as resolved after that.

@Dreamy_Jazz replaced use of isRegistered with isNamed as part of T344722 to ensure real names are only used for non-temp registered users (as is the intention).

I think that should have stayed with isRegistered. Temp users don't have real names (and on many wikis, including Wikimedia wikis, no one has) but the code falls back to the username, and this is the value that's used in the From: field and the subject of the email. So e.g. currently the email subject will end with (from 1.2.3.4) rather than (from ~2024-12345 at 1.2.3.4) which seems more helpful.

It's a very minor thing because the realname/username can be overridden via a form field anyway, but including temp users in the $fromName block seems like the right thing to me.

I cam here to close this ticket and see @Tgr's comment. I don't have any intuition about this personally -- so, perhaps @Dreamy_Jazz can weigh in here as the author of the original patch.

Some relevant workflows for the Stewards:

  • A named account is facing an IP block/is requesting IP block exemption
    • IP information is necessary here, and we'll have to guide them through obtaining it themselves if not provided by the form.
    • Username information is necessary here, and we'll have to ask for it if not provided by the form.
  • A logged out or temporary account user is facing an IP block
    • IP information is necessary here, and we'll have to guide them through obtaining it themselves if not provided by the form.
    • The temporary account name would be helpful in determining if the user is the target of the block or not. However, we can't exempt temporary accounts from IP blocks, so it's not required for handling the request.
  • A named account is appealing a global lock/block of the named account
    • IP information is useful to check for continued abuse, but is not required.
    • Username information can not be determined automatically for globally locked accounts, but is necessary to handle the request.
  • A temporary account user is appealing a global block of the temporary account
    • IP information is useful to check for continued abuse, but is not required.
    • Username information is necessary here, and we'll have to ask for it if not provided by the form.
  • Other requests, such as reports of compromised accounts, UCoC violations, etc
    • IP information is not required.
    • Requesting username is useful, but generally not required.

tl;dr: yes, I think including the temporary account name in the subject would be more useful. Allowing them to not include the IP is fine, we have an introduction explaining why we need the IP for a reason.

Change #1085600 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/extensions/ContactPage@master] Include names for temporary accounts as well on the subject line

https://gerrit.wikimedia.org/r/1085600

Change #1085600 merged by jenkins-bot:

[mediawiki/extensions/ContactPage@master] Include names for temporary accounts as well on the subject line

https://gerrit.wikimedia.org/r/1085600

Given @AntiCompositeNumber's input and the merge of the patch to handle that, I am going to resolve this task.

kostajh subscribed.

Thanks everyone! Re-opening the task, so it can go through QA.

ssastry subscribed.
dom_walden subscribed.

If I set 'MustBeLoggedIn' => false I can go to the Special:Contact as a temporary user and submit the form with Include my IP address in this message checked. The emailed subject line is Contact message (from <temp user> at <IP>).

Test environment: local docker ContactPage 2.3 (5bd3706) 07:20, 27 November 2024.

kostajh claimed this task.