Page MenuHomePhabricator

Clearly reserve system variables in VariableHolder
Closed, ResolvedPublic


Currently we use some variables in AbuseFilterVariableHolder to store internal info about the context: in fact, we have a 'context' variable inside mVars which we sometimes write/read to (see grep). That variable is also included in $varBlacklist, which prevents the export of listed items. If a user sets a 'context' variable inside filters, it actually seems to work, however it won't be dumped in logs as T170504 will be implemented.
Since (at least to me) it's not totally clear how such variables work, and since for the mentioned task I'd also need a reserved one, my proposal is to clearly reserve some variables, i.e. throw an exception if the user tries to overwrite them. However, we first need a query to see if a variable named 'context' is ever defined somewhere. It shouldn't be difficult, we only need a query like SELECT af_id FROM abuse_filter WHERE af_pattern LIKE '%context :=%' . We may also rename 'context' to something more difficult to overwrite, e.g. 'af_system_context', but I'm afraid old filters may react badly like it happened in T191696.

Event Timeline

Change 424850 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Prevent the user from overriding blacklisted variables

Daimona triaged this task as Medium priority.
Huji changed the task status from Open to Stalled.Apr 9 2018, 1:14 AM
Huji added a subscriber: Huji.

We need a subtask for the search and modification of the existing filters with a "context" variable in WMF.

@Huji Do we need a proper subtask or is it fine to do everything here? Actually, we'd need queries for several open tasks, is there an 'official' way to ask for them or should we just wait?

That is what I asked of @Bawolff in one of the other tasks. He responded to my question partially and copied @APalmer_WMF from legal too. I think this whole discussion (of what is the proper way to ask for queries and to fix filters enmass) deserves a task of its own! I will copy all of you in a task I am about to create in a moment!

@Huji Yes please! It doesn't make much sense to just let requests sit until someone will notice them by chance.

The same problem goes for global_log_ids and local_log_ids. These variables are stored and used in a way that it doesn't conflict with real variables, but what if the code is changed? A slight change may made those variables overwrite user-set ones, so we'd better avoid any risk. Also, we're using an "internal storage" for "meta-data", which isn't that good practice.

Daimona changed the task status from Stalled to Open.May 7 2018, 9:16 AM

No query results, ready to go

Change 424850 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Prevent the user from overriding blacklisted variables

Daimona removed a project: Patch-For-Review.