Page MenuHomePhabricator

Prevent Echo Notification Blacklist is storing Zeros in the database
Closed, ResolvedPublic3 Story Points

Description

After T173475: Echo Notification Mute (Block List) can be bypassed by changing username was deployed. It was discovered T177825 that the block list was storing zeros (or a set of zeros) in the database.

Originally it was assumed this was a result of the deployment and maintenance script T178313, but that does not appear to be the case.

Here is a query to see the rows that are bad:

SELECT * FROM user_properties WHERE up_property = 'echo-notifications-blacklist' AND up_value REGEXP '^(0|0\n)+$';

Here is a query to determine the valid rows:

SELECT * FROM user_properties WHERE up_property = 'echo-notifications-blacklist' AND up_value REGEXP '^[^0](.|\n)+$';

The number of bad rows has been increasing over time.

There are a significant amount of rows that are an empty string:

SELECT * FROM user_properties WHERE up_property = 'echo-notifications-blacklist' AND up_value = '';

However, these rows existed before T173475.

Ideally, if the user does not have a echo notification blocklist, then the row should not exist at all (i.e. null)


Acceptance criteria

  • Create a Phab ticket with info/steps on how to fix this
  • Decide the urgency of fixing this

Event Timeline

dbarratt created this task.Oct 18 2017, 4:27 PM
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptOct 18 2017, 4:27 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I will stand by, because I think I gave you all info you needed (which hopefully was useful). I will require the deletion of those files I generated after this is fixed; and of course you have my offering of anything that I could do to help.

If this is not causing any errors or problems, then I do not believe it is worth addressing at this time. @kaldari , thoughts?

dbarratt removed dbarratt as the assignee of this task.Oct 18 2017, 5:45 PM

If this is not causing any errors or problems, then I do not believe it is worth addressing at this time. @kaldari , thoughts?

It isn't, but it's possible/likely that those people previously had actual users on their blacklist in place of those zeroes, and that that data was lost. It may or may not be recoverable, but it would be good to make sure that such data loss does not continue.

It isn't, but it's possible/likely that those people previously had actual users on their blacklist in place of those zeroes, and that that data was lost. It may or may not be recoverable, but it would be good to make sure that such data loss does not continue.

According to T178313, these people did not have actual users on their blacklist (unless the analysis by @jcrespo and myself is inaccurate).

SELECT count(*) FROM user_properties WHERE up_property = 'echo-notifications-blacklist' AND up_value REGEXP '^(0|0\n)+$'; currently returns 1671 on English Wikipedia. Let's keep an eye on that number and see if it changes.

Unless you are saying they had a valid list, and that list has been lost at some point in the last week? (and not specifically after deployment?)

SELECT count(*) FROM user_properties WHERE up_property = 'echo-notifications-blacklist' AND up_value REGEXP '^(0|0\n)+$'; currently returns 1671 on English Wikipedia. Let's keep an eye on that number and see if it changes.

It has been steadily increasing every time I've checked it.

SELECT count(*) FROM user_properties WHERE up_property = 'echo-notifications-blacklist' AND up_value REGEXP '^(0|0\n)+$'; currently returns 1671 on English Wikipedia. Let's keep an eye on that number and see if it changes.

That's more than when I first found the issue on Saturday. It was in the 1300s then.

kaldari triaged this task as High priority.Oct 18 2017, 7:02 PM

It's 1674 now, so it's definitely increasing.

Also, there are currently 11012 entries set to ''.

It's 1674 now, so it's definitely increasing.

Yeah it's 1,754 now. How should this task be prioritized?

Let's get this into our Sprint 8 (Oct 25 - Nov 7) or Sprint 9 (Nov 8 - 21) following the Interaction Timeline MVP.

However, if we realize this is more time sensitive, we can bump to before the Timeline.

TBolliger updated the task description. (Show Details)Nov 3 2017, 6:04 PM
TBolliger updated the task description. (Show Details)
dbarratt renamed this task from Investigate why the Echo Notification Blacklist is storing Zeros in the database to Prevent Echo Notification Blacklist is storing Zeros in the database.Nov 3 2017, 6:08 PM
dbarratt set the point value for this task to 3.

Since this might require a huge amount of debugging, we'll instead prevent zero's from being written, if the number stops increasing then we will have resolved the issue.

kaldari added a comment.EditedNov 3 2017, 6:48 PM

Current number of accounts with zeros on en.wiki: 2378.

dbarratt claimed this task.Nov 13 2017, 3:53 AM
dbarratt moved this task from Ready to In progress on the Anti-Harassment (AHT Sprint 9) board.
dbarratt added a subscriber: dmaza.Nov 13 2017, 3:19 PM

@dmaza you were right about the intval

php > print_r(array_map( 'intval', explode( "\n", '') ));
Array
(
    [0] => 0
)
php > print_r(array_map( 'intval', explode( "\n", ' ') ));
Array
(
    [0] => 0
)
php > print_r(array_map( 'intval', explode( "\n", '0') ));
Array
(
    [0] => 0
)

So people who have an empty string (or just a space for that matter) are getting converted to 0.

Change 391032 had a related patch set uploaded (by Dbarratt; owner: Dbarratt):
[mediawiki/extensions/Echo@master] Prevent loading or saving of zeros in the database.

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

Change 391032 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Prevent loading or saving of zeros in the database.

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

dbarratt closed this task as Resolved.Nov 20 2017, 8:11 PM
dbarratt moved this task from Review to Done on the Anti-Harassment (AHT Sprint 9) board.