Page MenuHomePhabricator

AbuseFilter tracks disabled actions but doesn't report them
Closed, ResolvedPublic

Description

When a user makes a filter execute an action which is disabled in the configuration, it is not added to filter without any warning, silently. In the following code, $deadActions var is populated but then it's never actually used.

includes/Views/AbuseFilterViewEdit.php$220
global $wgAbuseFilterActions;
$deadActions = [];
$actionsRows = [];
foreach ( array_filter( $wgAbuseFilterActions ) as $action => $_ ) {
  // Check if it's set
  $enabled = isset( $actions[$action] ) && (bool)$actions[$action];

  if ( $enabled ) {
    $parameters = $actions[$action]['parameters'];

    $thisRow = [
      'afa_filter' => $new_id,
      'afa_consequence' => $action,
      'afa_parameters' => implode( "\n", $parameters )
    ];
    $actionsRows[] = $thisRow;
  } else {
    $deadActions[] = $action;
  }
}

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 24 2018, 5:12 PM
Huji added a subscriber: Huji.EditedFeb 24 2018, 7:25 PM

What do you think should be done here? Log them with wfDebug() or report it in the Special:AbuseLog entry itself (e.g. "... Actions taken: Tag; Actions not available: Block; Filter description ....")?

I haven't examine yet what happens exactly. Looking at the snippet, I suppose that the filter is simply saved without the action (ie. with no record in the database), so it cannot tracked down later. Some warning before or after saving it should work here.

I haven't examine yet what happens exactly. Looking at the snippet, I suppose that the filter is simply saved without the action (ie. with no record in the database), so it cannot tracked down later. Some warning before or after saving it should work here.

Right. From my POV, we should add a warning message similar to the one for T173947. The only point is: should we prevent saving (=true error) or save with a warning? In any case we need to do it before writing anything in the master.

Daimona triaged this task as Normal priority.Mar 11 2018, 10:05 AM

Change 430631 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove unused code

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

Daimona claimed this task.May 3 2018, 5:41 PM

The patch is there, however I removed the code per comment on gerrit: AFAICS, $deadActions isn't related to config-disabled actions.

Change 430631 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Remove unused code

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

Daimona closed this task as Resolved.May 3 2018, 6:34 PM