Page MenuHomePhabricator

Fix string cast behaviour for lists
Open, LowPublic

Description

Right now, casting an array to string gives the following result:

list := ['a','b','c'];
string(list) == 'a\nb\nc\n'

While it makes sense to implode it with linebreaks, it's wrong to add a trailing one. The fix itself is pretty straightforward, but the reason I'm opening this task is to have a view of how many filters rely on this. So, I'm kindly asking of a database query to find every usage of string() function. Then, depending on the # of matches (refining the search if necessary), we'd need to determine how many filters actually rely on this and fix them.

Event Timeline

Change 421695 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove the last linebreak from array to string conversion

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

Well, actually I just realised that the impact isn't limited to the "string" function: it touches every cast from list to string, even the ones internal to the code made using toString(). Another good reason to gather data and prevent regressions.

Daimona triaged this task as Medium priority.

This is what we need to do/examine prior to merging:

  • Query all existing filters for uses of string( (they shouldn't be too much)
  • Query all existing filters for strings ending with \n. It should be enough to check for \\n['"] (I don't expect too many matches)
  • Check PHP functions calling toString (on lists), ensuring that nothing will change.

I'll give a look at the last point, while I kindly ask someone with enough rights for a summary of the first 2: total # of matches, then we'll see how to proceed.

I couldn't find anything that will fail in the code itself. We only need the 2 queries.

@Bawolff since you have helped with other DB queries in the past, I am adding you here. What is the right approach though? Is there a project tag for DB requests like this? And also, should this be converted to a Security task since some of the filters that string() may not be public? And lastly, once we identify them, who are the group of (super)users who can modify these filters enmass?

My initial reaction would be, that whether or not an abuse filter pattern contains a cast to string is not the thing that hidden filters are trying to prevent the public from finding out. So if its just a list of filters that have that operation (with no other details), I don't think that would have to be secret.

I agree with @Bawolff's remark. As far as we only make a list of filters using string( or \\n['"] we won't have security troubles. As for who should edit those filters, I'd also like to know. I could do it myself but I'm not even close to having the needed rights.

Now that we have the results, we can move on. All filters listed there should be manually checked, and I'll do that if and once I'll get global abusefilter helper right.

I hope to start today with the review of existing filters, but first I need to determine which variables/functions will be affected by this change. Looking at a sample of 3-4 enwiki filters, it seems that this change will actually affect some of them, so I'll start to write down a list of needed changes, although it's not clear how are we supposed to execute them.

I started writing the list. However, I found some use cases where \n is used as a trick to detect the end of an element, and other stuff which should be replaced with array-specific functions (https://gerrit.wikimedia.org/r/#/c/424298/) once ready. We should probably wait for it before going on.

Change 421695 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Remove the last linebreak from array to string conversion

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

Daimona lowered the priority of this task from Medium to Low.

While I'd love to implement this change, this is almost impossible due to current usage.

Change #421695 abandoned by Daimona Eaytoy:

[mediawiki/extensions/AbuseFilter@master] Remove the last linebreak from array to string conversion

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