Page MenuHomePhabricator

Dynamic variable names shouldn't be allowed
Closed, ResolvedPublic

Description

As noted in T214674#5396912, AbuseFilter currently allows declaring dynamic variable names with setter functions, e.g.

set_var( page_title + '_my_string' + ( page_namespace == 0 ? 'main' : 'foo' ), 'myVarValue' )

is valid syntax. However, the AF language is meant to be simple, and dynamic variable names are something complicated to deal with even in standard programming languages.
So my proposal is to only allow a literal string as first argument to set_var.

Event Timeline

Daimona created this task.Aug 6 2019, 5:41 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 6 2019, 5:41 PM
Krinkle added a subscriber: Krinkle.EditedAug 6 2019, 5:46 PM

+1, does not seem like a worthwhile feature for the cost and complexity it requires in the critical path of saving edits.

It doesn't enable or improve anything for the end-user (catching abusive contributions). Anything it would help to catch, one can also catch as easily (or easier) without using this feature.

Change 528540 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Ban variable variables

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

Daimona claimed this task.Aug 6 2019, 7:01 PM
Huji added a subscriber: Huji.Aug 7 2019, 10:53 PM

@Daimona, I think we should run a query to make sure no WMF wiki already uses this feature, before disabling it. Can you take the lead on that subtask?

@Daimona, I think we should run a query to make sure no WMF wiki already uses this feature, before disabling it. Can you take the lead on that subtask?

Is any special access needed to be able to query the abuse_filter tables?

Huji added a comment.Aug 8 2019, 12:36 PM

You can query public filters like this but (a) you cannot query private filters, and (b) this would require running the query for hundreds of wikis one at a time.

Therefore, we have historically used the help of those WMF folks with DB access in situations like this. We will give them the query in a private Phab Task, they will run it against the DB (hence the result also including private filters), and then we modify the filters as needed. In wikis where Global Sysops can edit the filters, I do the edits myself (I am a Global Sysop solely for that reason). In wikis where Global Sysops do not have this right (which includes most major Wikipedias like English and German) we ask the local admins to make the edits.

I am doubtful that set_var is used on more than a handful of filters overall, and I am doubtful that any of them uses it with dynamic variable names. But better safe than sorry.

Huji renamed this task from Variable variables shouldn't be allowed to Dynamic variables shouldn't be allowed.Aug 8 2019, 12:36 PM
Huji updated the task description. (Show Details)
Huji added a comment.Aug 8 2019, 12:39 PM

Here is the thing tho: if we only want to allow literal strings as the first parameter, then what is the point of having set_var at all? We already can use the variable := value syntax for that.

I think we should allow set_var to accept a string as the first parameter, but not allow it to accept an "expression" as the first parameter.

So this would not be allowed:

set_var( page_title + '_my_string' + ( page_namespace == 0 ? 'main' : 'foo' ), 'myVarValue' )

But this would:

var_name := page_title + '_my_string' + ( page_namespace == 0 ? 'main' : 'foo' );

set_var(  var_name, 'myVarValue' )

@Daimona, I think we should run a query to make sure no WMF wiki already uses this feature, before disabling it. Can you take the lead on that subtask?

Yeah, probably. I can do next week, or feel free to open it if you want.

[...] and then we modify the filters as needed. [...] Global Sysops [...]

I suggest that we try to restart the discussion about the global abusefilter managers group on meta. I was thinking to do that next week, and maybe write on tech news. ATM there are several tasks for which we'd need those rights.

I am doubtful that set_var is used on more than a handful of filters overall,

Agreed

and I am doubtful that any of them uses it with dynamic variable names. But better safe than sorry.

Ditto

Here is the thing tho: if we only want to allow literal strings as the first parameter, then what is the point of having set_var at all? We already can use the variable := value syntax for that.

In fact, IMHO there's no point. It'd just be an alternative syntax for those who prefer to write assignments like that. Removing it altogether would be even more painful.

I think we should allow set_var to accept a string as the first parameter, but not allow it to accept an "expression" as the first parameter.

That's what the patch does.

But this would:

var_name := page_title + '_my_string' + ( page_namespace == 0 ? 'main' : 'foo' );
set_var(  var_name, 'myVarValue' )

It's not allowed with my patch, and that's what I'd like to drop support for.

Huji added a comment.Aug 8 2019, 4:49 PM

I suggest that we try to restart the discussion about the global abusefilter managers group on meta. I was thinking to do that next week, and maybe write on tech news. ATM there are several tasks for which we'd need those rights.

Can you please create a parent task for them and/or a tag? It'd be best to have an easy way to keep track of these. Also, can you please subscribe me to them? Perhaps I can use my Global Sysop right for this purpose for now?

I am in favor of having the discussion restarted. Please {{ping}} me there too.

I suggest that we try to restart the discussion about the global abusefilter managers group on meta. I was thinking to do that next week, and maybe write on tech news. ATM there are several tasks for which we'd need those rights.

Can you please create a parent task for them and/or a tag? It'd be best to have an easy way to keep track of these. Also, can you please subscribe me to them? Perhaps I can use my Global Sysop right for this purpose for now?
I am in favor of having the discussion restarted. Please {{ping}} me there too.

Created T230263. I'll add other subtasks in case I forgot any of them. And I'm also going to restart the rfc.

Huji renamed this task from Dynamic variables shouldn't be allowed to Dynamic variable names shouldn't be allowed.Aug 11 2019, 1:50 AM
Huji updated the task description. (Show Details)

You can query public filters like this but (a) you cannot query private filters, and (b) this would require running the query for hundreds of wikis one at a time.
Therefore, we have historically used the help of those WMF folks with DB access in situations like this. We will give them the query in a private Phab Task, they will run it against the DB (hence the result also including private filters), and then we modify the filters as needed. In wikis where Global Sysops can edit the filters, I do the edits myself (I am a Global Sysop solely for that reason). In wikis where Global Sysops do not have this right (which includes most major Wikipedias like English and German) we ask the local admins to make the edits.
I am doubtful that set_var is used on more than a handful of filters overall, and I am doubtful that any of them uses it with dynamic variable names. But better safe than sorry.

So I guess there is nothing I can do to help at this point, since {P8894} is private

You can query public filters like this but (a) you cannot query private filters, and (b) this would require running the query for hundreds of wikis one at a time.
Therefore, we have historically used the help of those WMF folks with DB access in situations like this. We will give them the query in a private Phab Task, they will run it against the DB (hence the result also including private filters), and then we modify the filters as needed. In wikis where Global Sysops can edit the filters, I do the edits myself (I am a Global Sysop solely for that reason). In wikis where Global Sysops do not have this right (which includes most major Wikipedias like English and German) we ask the local admins to make the edits.
I am doubtful that set_var is used on more than a handful of filters overall, and I am doubtful that any of them uses it with dynamic variable names. But better safe than sorry.

So I guess there is nothing I can do to help at this point, since P8894 is private

See T230262#5407225

Huji closed this task as Resolved.Aug 11 2019, 3:53 PM
Huji removed a project: Patch-For-Review.

Change 528540 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Ban variable variables

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