I propose to implement a system that warns a user who is making an edit in the AbuseFilter that may cause false positives in a dangerous way. Like regex mistakes that may cause empty matches or match infinitely.
Kind regards.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Return warnings from the parser, add warning for catch-all regexps | mediawiki/extensions/AbuseFilter | master | +137 -39 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Daimona | T264768 Avoid dangerous AbuseFilter edits | |||
| Resolved | Daimona | T187686 Write an Ace worker for AbuseFilter and add clientside validation |
Event Timeline
Comment Actions
The property of being a "dangerous" filter is undecidable due to Rice's theorem (I believe), so there's no way we can fully implement this. We might add some common checks (e.g. for regexps matching the empty string), but that wouldn't be complete. And even then, disallowing the edit is out of question. We should only show a warning to the user, but that's blocked on T187686, which seems non-trivial.
Comment Actions
@Daimona: Does that mean this task should be either "declined", or "stalled" and T187686 should be marked its subtasks? It's unclear to me what this is stalled on...
Comment Actions
It's unclear to me as well. It depends on what exactly is the scope of this task. More precisely:
- If it's a generic "detect and disallow dangerous edits", then it can be closed as declined per Rice's theorem + WONTFIX the "disallow" part
- If it's just "warn about regexps that match everything", then this can be reopened with T187686 as the blocking subtask
- If it's a generic, umbrella-like "add detection for possibly dangerous edits", then it can be either closed as invalid per the task being an umbrella without precise actionables, or specific subtasks can be created for the single features being requested (if any), and these task would in turn be blocked on T187686.
My opinion is that (2) seems the best option, but I'd like to hear from @SRuizR what kind of "errors" we should warn about.
Comment Actions
Aside from regexp patterns matching blank (or matching everything), do we have any other "dangerous" edits in mind? Have they ever happened, and if so, have they happened frequently enough to be a concern?
Also, why bother? If a filter matches too many edits, it'll quickly get throttled anyway.
Comment Actions
The only thing I thought about was the regex empty match, but there may be other things. Anyway, I think Daimona's option (2) is the best thing to avoid incidents.
Comment Actions
I think this could be merged with T187686 because I think it's the only possible thing that could be made.
Comment Actions
Not really. These are really two different things. If this couldn't be made, the task would be declined.
Comment Actions
Some comments as a global abuse filter writer.
- While I can write a global abuse filter it is not readily able to be checked against past edits in the meta log of global hits. I know that I have written filters that have regex errors, and in among the noise of meta's abuselog haven't been able to identify low level false positives. Those filters with large regex errors show up pretty quickly.
- When adding to an existing filter, you cannot test it from right there within the filter. You either have to go to another page with a copy and paste; or save it and go look at the consequences. Many a time when I make small change I will make the edit and go and check and test, presuming that I haven't made and missed a typo.
So I suppose that the current design is more purposed to run the risk to make an edit, especially to a regex and see what it does, rather than have one make edits and check the outcome prior to committing the filter. So even the ability to have a temporary save of a filter and check of a regex would be useful.
Comment Actions
Change 650530 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Return warnings from the parser, add warning for catch-all regexps
Comment Actions
Change 650530 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Return warnings from the parser, add warning for catch-all regexps
Comment Actions
I'm going to close this as resolved since the regex-matches-all warning was implemented, and that seemed to be the only clear actionable from this task. Similar feature requests can be filed as separate tasks.