Page MenuHomePhabricator

Investigate: Confirm our logging expectations for AbuseFilter/CheckUser Temporary Accounts logs when using protected variables
Closed, ResolvedPublic

Description

T385687: Investigate: How should the AbuseFilter variable `user_unnamed_ip` be restricted proposes that extensions should be allowed to declare their own restrictions on protected variables. This is to support the centralization of IP access within the CheckUser extension. However, no user-facing interface exposes this information. Please investigate whether or not this is important to expose. From T385687#10579417:

One thing that occurred to me - should we log that access was affected? A protected variable being viewed w/o these CU constraints means something different than if CU isn't enabled and that wouldn't be clear just by looking at AF logs. Additionally, if we're going to redirect as we're doing now, it's also not clear from AF logs that some of the logs also exist in CU's logs instead. This is an existing problem but I guess now's the time to re-surface it. I don't think this investigation is blocked on this question, as it can be answered as part of the logging refactoring task.

Additionally, as of T387328: Refactor CheckUser ProtectedVarsAccessLogger logging out of AbuseFilter, all protected vars logs should still be funneled to CheckUser (as only one protected variable exists, user_unnamed_ip and CU wants to centralize IP logs). However, when IPReputation variables are introduced, those logs should continue to remain in AbuseFilter as they are not considered sensitive the way IPs are. Please investigate how we expect logging to behave when we want different outcomes per variable.

Current thoughts (please update as necessary):

  • If we don't record what variable is being viewed (afaik this is not a legal requirement), are we ok with duplicate logs? If so we could maintain a full list in AF and a subset in CU with CU managing what logs it wants to keep
  • If we do record what variables are being viewed...we still might want to maintain duplicate logs if it's okay as there would be no other way to get a full list of who viewed protected variables otherwise

Per comment, this is split off from other tasks supporting T380920: Remove the AbuseFilter protected variables preference gate as the status quo seems acceptable for now and this is not currently considered blocking for temporary accounts. However, it may be blocking for IPReputation variables.

Related Objects

Event Timeline

Based on Samuel's note here we should log when a user views an AbuseFilter log with a protected variable containing IP reputation data.

Based on Samuel's note here we should log when a user views an AbuseFilter log with a protected variable containing IP reputation data.

I've asked @sguebo_WMF and @EMagallanes for clarification on this, though, in this comment

Based on Samuel's note here we should log when a user views an AbuseFilter log with a protected variable containing IP reputation data.

I've asked @sguebo_WMF and @EMagallanes for clarification on this, though, in this comment

Sguebo_WMF has recommended we continue to log when a user sees IP reputation protected variables.

STran renamed this task from Investigate: Should AbuseFilter logs also log modifications to protected vars access requirements? to Investigate: Confirm our logging expectations for AbuseFilter/CheckUser Temporary Accounts logs when using protected variables.Mar 20 2025, 10:46 AM

Talked to @kostajh to confirm and it sounds like it's up to us as long as we log somewhere per Legal and the following behavior will work:

  • We should centralize the IP view logs as we're doing now. With the expansion of protected variables, we'll need to only route the ones that use user_unnamed_ip to the temp account logs
  • If not too difficult, we should update the log to mention what protected variable is being viewed so that it's explicit that the logs being routed to the temp account logs are due to the user_unnamed_ip variable

cc @Dreamy_Jazz

Talked to @kostajh to confirm and it sounds like it's up to us as long as we log somewhere per Legal and the following behavior will work:

  • We should centralize the IP view logs as we're doing now. With the expansion of protected variables, we'll need to only route the ones that use user_unnamed_ip to the temp account logs
  • If not too difficult, we should update the log to mention what protected variable is being viewed so that it's explicit that the logs being routed to the temp account logs are due to the user_unnamed_ip variable

cc @Dreamy_Jazz

I would agree with implementing this behaviour.

I've created T389613: Only divert `user_unnamed_ip` protected variable views to CheckUser logs and T389614: Update protected variable view log copy to reflect what protected variable(s) are being viewed. I put T389613 in our current sprint as I would consider it a blocker to adding new variables as we would divert too many logs and left T389614 since this was less critical although I think it would be nice to do before so that we don't have to maintain a legacy log line (as we aren't passing in any variable parameters atm).