Page MenuHomePhabricator

AbuseFilter needs better tag name validation
Closed, ResolvedPublic

Description

Various pieces of the tagging system will break if the tag name contains slashes or commas (e.g. T27151), so that should be checked for.

Recent changes have added the necessary validation to core, so it just needs factoring out there then calling here.

Details

Related Gerrit Patches:
mediawiki/extensions/AbuseFilter : masterImprove tag name validation
mediawiki/core : masterAdd | to error message about invalid change tag characters
mediawiki/core : masterFactor out changetag name validation check

Event Timeline

Anomie created this task.Mar 17 2015, 1:01 PM
Anomie claimed this task.
Anomie raised the priority of this task from to Needs Triage.
Anomie updated the task description. (Show Details)
Anomie added a project: AbuseFilter.
Anomie added a subscriber: Anomie.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 17 2015, 1:01 PM

Change 197314 had a related patch set uploaded (by Anomie):
Factor out changetag name validation check

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

Change 197315 had a related patch set uploaded (by Anomie):
Improve tag name validation

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

Aklapper triaged this task as Medium priority.Mar 18 2015, 11:17 AM

Change 197314 abandoned by Anomie:
Factor out changetag name validation check

Reason:
If someone else wants to pick this up, feel free.

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

Change 197315 abandoned by Anomie:
Improve tag name validation

Reason:
If someone else wants to pick this up, feel free.

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

Anomie removed Anomie as the assignee of this task.Dec 29 2016, 3:04 AM

Change 197314 restored by Mattflaschen:
Factor out changetag name validation check

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

Change 197315 restored by Mattflaschen:
Improve tag name validation

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

Change 197314 merged by jenkins-bot:
[mediawiki/core@master] Factor out changetag name validation check

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

On https://gerrit.wikimedia.org/r/#/c/197314/ , I suggested that we change it so all tags must be:

2. One of:
2a. canAddTagsAccompanyingChange (explicit tags + subset of allowed software, also implies that it exists)
2b. Already a defined AbuseFilter tag (this catches the 'already exists' possibility)
2c. canCreate.

I wanted to see if users habitually re-use software-defined tags for their own AbuseFilter tags, and if so how we might (or might not) accommodate it.

I wrote a script to investigate, P5491, which checks for whether software-defined tags are used, and whether the filter is enabled.

Full output: P5492

Grepped for tag names:

grep ',' extension-defined-2017-05-26.txt|grep -Ev 'TorBlockHooks::onListDefinedTags|MassMessageHooks::onRegisterTags|VisualEditorHooks::onListDefinedTags|MobileAppHooks::onListDefinedTags|MobileFrontendHooks::onListDefinedTags|ContentTranslationHooks::registerTags|MWOAuthHooks::getUsedConsumerTags|WikimediaEventsHooks::onListDefinedTags|WikiLoveHooks::onListDefinedTags|Hooks::onListDefinedTags|1 => false|\),'|arc paste

1ukwiki: 3 => 'visualeditor-needcheck',
2ukwiki: 2 => 'visualeditor-needcheck',

(Sorry, I should have made it possible to grep through properly but by that time it had already been running).

So 'visualeditor-needcheck' is used in one or more enabled AbuseFilter filter on one wiki, but it doesn't seem like this practice is widely used at all. IMHO, this tag is actually a good indication of why it's not good practice unless the extension opts into it (this particular one should be used for round-trip issues detected by VE itself, and wikis can choose another tag for "VE edit with possible issues" with their own criteria).

Mattflaschen-WMF added a comment.EditedMay 27 2017, 1:18 AM

So I don't think we need to whitelist any additional tags via ChangeTagsAllowedAdd (that tag will keep working on that specific wiki due to the general grand-fathering for tags already being used by AbuseFilter).

Change 359085 had a related patch set uploaded (by Mattflaschen; owner: Mattflaschen):
[mediawiki/core@master] Add | to error message about invalid change tag characters

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

Change 359085 merged by jenkins-bot:
[mediawiki/core@master] Add | to error message about invalid change tag characters

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

Change 197315 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Improve tag name validation

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

matmarex closed this task as Resolved.Jun 26 2017, 5:39 PM
matmarex removed a project: Patch-For-Review.