Wikimedia Bugzilla is closed!

Created attachment 9127 [details]
Provided a comment for existing blacklist.

Provided a comment for existing blacklist. Without comment it is not clear which characters are blacklisted, because they are represented by codes. With my comment it is clear that all the blacklisted characters are slash-alike.

I can easily prepare a patch if author/maintainer of the extension let me know the preferred implementation. It could be:

1. Making $character_blacklist a public member.
2. Adding a public static method, something like extendBlacklist().
3. Introduce a global variable so user can set it before including the extension.

Comment on attachment 9127 [details]
Provided a comment for existing blacklist.

Committed in r98457


Using a global is probably the most sane way, so people can override it in their LocalSettings, rather than having to potentially mess about with changing the extension directly.

Note, I don't think the extension has an active maintainer

Created attachment 9134 [details]
Customable blacklist.

This is the patch. $wgAntiSpoofBlacklist is a global array. It can be set before including AntiSpoof.php, or modified after.

Ping.

Please either apply the patch or say it will not be applied.

Van, I asked Brion, who is the listed maintainer of the extension:

<sumanah> brion: who is maintaining the AntiSpoof extension these days? it looks like Van de Bugger would be interested in taking over as maintainer, judging by his recent patches in Bugzilla
<brion> sumanah, i would be happy to see that happen. i haven't touched it much in a few years, if nobody else has been active on it then awesome :D

Judging from https://www.mediawiki.org/w/index.php?title=Special:Code/MediaWiki&path=%2Ftrunk%2Fextensions%2FAntiSpoof%2F the other people who have touched it somewhat recently are soxred93, iAlex, Platonides, Daniel Friesen (Dantman), and Chad.  I've asked Platonides and Daniel to take a look at this patch, but I'm sorry to say that I cannot guarantee a time or date by which it'll be reviewed.

Van de Bugger: so, you might wish to reach out to those committers to ask for a review and to ask one of them to take over maintainership.  And if you're interested in maintaining the extension and ready to do so, you should apply for commit access via https://www.mediawiki.org/wiki/Commit_access_requests#Requesting_commit_access .

(In reply to comment #4)
> Created attachment 9134 [details]
> Customable blacklist.
> 
> This is the patch. $wgAntiSpoofBlacklist is a global array. It can be set
> before including AntiSpoof.php, or modified after.
>

Using isset() like this makes the extension vulnerable to register_globals. You should unconditionally define it (with the default values) and users can extend it by doing

$wgAntiSpoofBlacklist[] = 0x1234;

Or can overwrite it entirely by doing

$wgAntiSpoofBlacklist = array( 0x1234 );

The is_array() check should be moved into checkUnicodeString() and die() should be replaced with throwing a MWException.

> You should unconditionally define it (with the default values)…

No problem.

> The is_array() check should be moved into checkUnicodeString() and die() 
> should be replaced with throwing a MWException.

I tested both:

    throw new MWException( 'message...' );

and

    throw new ErrorPageError( 'page-title-msg-id', 'msg-id' );

and do not like both.

The first one causes special page with stack backtrace, which is useless, because shows where the bug was found, but does not shows where the bug is. 

The second one allows localized messages, which is probably good.

However, the biggest issue is that in both cases error is shown only when a user fills the registering form and press submit. Too late to my taste. It would be better if configuration errors re reported instead of *any* page. `die' in `AntiSpoof.php' provided such behaviour, so site admin will notice the error *very* soon. In the proposed approach the error will see only user trying to register.

However, I do not insist on dying in `AntiSpoof.php'.

Created attachment 9528 [details]
Custom blacklist, attempt #2.

1. $wgAntiSpoofBlacklist is defined unconditionally.
2. No `die', exception thrown in `checkUnicodeString'.

Hmmm... adding $wgAntiSpoofBlacklist as a feature sounds like a good feature addition.

However I don't know about adding the default list of characters to it. Those seam like characters we absolutely don't want to see, not ones that should suddenly start working if someone happens to use `$wgAntiSpoofBlacklist = array( ':' );` inside their config emptying out the original array's contents.

How about keeping that built in character list as it is, but adding the $wg stuff in addition to it. Either by merging them together or duplicating the test.

Or is there a good reason people would want to purposefully remove the default rules?

> Those seam like characters we absolutely don't want to see,…

Why? AntiSpoof is an extension, optional by nature. If those are characters you are /absolutely/ do not want to see, why they are not blacklisted in core? All these blacklisted characters are allowed in MW out-of-the-box, and disabled only if AntiSpoof is installed and enabled.

> How about keeping that built in character list as it is, but adding the $wg
stuff in addition to it.

IMHO, it complicates implementation with no real benefit.

Ping.

The patch looks fine, however I see no need in localisation of exception messages. Do you have commit access now or should I commit it?

Max, I think you should just go ahead and commit it.

Max: Did you have time to commit the patch in comment 10?

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

Change 47902 merged by jenkins-bot:
Bug 31256: a way to customize the AntiSpoof blacklist

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