Page MenuHomePhabricator

Investigate: Which extensions save IP addresses
Closed, ResolvedPublic

Description

See also T356430: Investigate: Where does MW core save IP addresses.

This is spun out from T336187: [S] Investigate: Creating temp user on non-EditPage actions, as explained in T336187#9341749:

To guard against accidentally creating an IP actor (because IP actors will now pass permissions checks), do the following:

  • [...]
  • Look for workflows that save the IP address other than through the actor table (e.g. like Flow: T345578#9290889)
Background

If temporary accounts are enabled, the ActorStore throws an error on attempting to create an actor with an IP address. This means that a revision authored by an IP actor won't be able to be saved, and more generally any action that creates an IP actor should not be possible.

We found an exception to this in the Flow extension. Flow saves the comment, along with the IP address (as the author) in a local Flow table. It then attempts to create the IP actor in core but fails - but the damage is already done. The IP address saved in the Flow table will appear in the UI. (More details in T345578#9290889.)

In this task we want to look for other examples of extensions that save IP addresses.

What to investigate
  • Which extensions save IP addresses?
  • Do these cases need fixing, and if so how (or who can tell us how)?

Event Timeline

I performed a search from the string 'ip' in any (WMF deployed) file containing the string 'tables'.

Search: https://codesearch.wmcloud.org/deployed/?q=ip&files=.*tables.*&excludeFiles=

I then read through the results looking for table columns containing 'ip' (ignoring core) and found possible hits in:

  • CheckUser
  • AbuseFilter
  • Echo
  • Flow
  • SecurePoll

...which we should look into.

Tchanders updated the task description. (Show Details)
ExtensionDoes it save IP addresses? What for?What should we do?
CheckUserYes. This is the one place where we'll be storing IP addresses, so this is fine.No action needed.
AbuseFilterThis stores IP addresses in abuse_filter_log.afl_ip if $wgAbuseFilterLogIP is true. This happens in WMF production: we currently store IP addresses in the AbuseFilter tables.Needs investigation.
FlowCommenting as a logged out user does not create a temporary account. Instead it saves the IP address in flow_revision.rev_user_ip and displays it in the UI. Does this even if IP actor creation in core errors out. More details: T345578#9290889.Flow needs to be disabled or updated. Growth-Team is working on this in T346108.

To do next

  • Echo
  • SecurePoll
ExtensionDoes it save IP addresses? What for?What should we do?
CheckUserYes. This is the one place where we'll be storing IP addresses, so this is fine.No action needed.
AbuseFilterThis stores IP addresses in abuse_filter_log.afl_ip if $wgAbuseFilterLogIP is true. This happens in WMF production: we currently store IP addresses in the AbuseFilter tables.Needs investigation.

AbuseFilter purges IP addresses from logs, similarly to CheckUser. Frequency is controlled by the config AbuseFilterLogIPMaxAge, which is set to 3 months by default (and on production).

This does mean that AbuseFilter is an additional place where we store IPs, but it treats them the same as CheckUser, which is fine.

To do next

  • Echo

The column event_agent_ip holds the IP address of an anon user, obtained from their name. It won't be written too once temp accounts are deployed, since isRegistered will return false.

  • SecurePoll

SecurePoll is similar to CheckUser and AbuseFilter: it records IP addresses then purges them after 90 days. Configured here.

kostajh claimed this task.

Based on @Tchanders' notes here, I am resolving this task.