Currently, we have under review a patch to switch AbuseFilter to use Ace editor if CodeEditor is installed. We may take the opportunity and write a custom Ace worker to provide not only a syntax highlight but also a client-side syntax validation for filter rules (while leaving as it is the server-side one). This way the user doesn't need to click "check syntax", he is reported with row-by-row error messages, also a bit more explicative than the current ones. Moreover, we may add warnings and info. The first could be used e.g. to warn the user of a deprecated syntax (like for T173889), the latter to offer some performance tips, like the ones in the Rules format page.
I'm separating this from the syntax highlight itself since this is a more complex task.
Description
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Run real-time validation of rules with an Ace worker | mediawiki/extensions/AbuseFilter | master | +1 K -3 |
| 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
@matej_suchanek Yeah, definitely something we want to implement :-) Unfortunately I took a look at the workers some time ago but I'm afraid I'm not able to write one.
Another thing we could put as a warning: users are allowed to write filters like
1 == 1 &
And leaving a "&" without operators always return false. This behaviour might have its sense and could be here to avoid dealing with nasty situations and/or parser bugs, but we should at least make clear to the user that such condition will always return false. Due to this, I accidentally disabled a quite important itwiki filter for a month, noticed it by chance thanks to MusikBot reporting it as staled and it took me another 10 minutes to spot the error.
Another random idea for warnings: use them when a function taking x parameter is called with more than x parameters.
So I tried to do this today... Setting up the worker is not too hard, the only thing we need to pay attention to are paths: ace uses relative paths, and CodeEditor (and also our edit module) set the base path to extension/CodeEditor/modules, because that's where ace lives. But this new mode would live in AF/modules, so we need to change the path. But anyway, it shouldn't be super-hard to fix that.
The main problem is how to validate the filter code. I see two alternatives:
- Port the PHP parser to JS. That'd be a ridiculous waste of time, and hard to maintain.
- Do that via an API call to AbuseFilterCheckSyntax (like the "Check syntax" button does). However, we'd need the mw global to be available inside the worker, and I'm not sure if that's possible. I tried to play around a bit, and wasn't able to do that.
Change 647024 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Run real-time validation of rules with an Ace worker
So, the approach above works as expected, and if we're just looking at that, it can be merged as-is. However, due to the use of Workers (of which I really have little experience), we need to bootstrap a lot of ace functionalities, which results in roughly 1600 lines of copypasta boilerplate. This is probably worsened by the ResourceLoader layer and the fact that most Ace scripts come from CodeEditor.
If we found a way to avoid that (or determine that it cannot be avoided), then this is done.
I believe this is not possible. That is also further complicated by the worker running in a blob, and us not having requirejs (because we use RL). The other workers (that come with ace itself) included in CodeEditor also have this boilerplate. So I think it would be a pointless effort to avoid the boilerplate, and the patch can be merged as-is.
Stub for Tech News:
The AbuseFilter edit interface now shows syntax errors while typing. This is the similar to JavaScript pages. It also displays a warning for regular expressions that match the empty string, and new warnings will be added. [[:phab:T187686|]]
Added a contextual double-AF update to Tech/News: https://meta.wikimedia.org/w/index.php?diff=20856647&oldid=20818615&title=Tech/News/2021/02&diffmode=source
Change 647024 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Run real-time validation of rules with an Ace worker