Page MenuHomePhabricator

AbuseFilter does not fail gracefully with bad regex's
Closed, ResolvedPublic

Description

Author: overlordq

Description:
See the ENWP helpdesk[1], if somehow an abuse filter is created with an invalid regex instead of throwing some sort of helpful error, an internal error is generated.

1 - http://en.wikipedia.org/wiki/Wikipedia:HELPDESK#Why_does_every_edit_I_try_to_do_create_an_exception_error.3F


Version: unspecified
Severity: major

Details

Reference
bz19203

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:43 PM
bzimport added a project: AbuseFilter.
bzimport set Reference to bz19203.

I believe this should be INVALID

Prior to the recent update there was no error checking on regex expressions at all. All bad regex returned false in all cases.

From now on filter editors are prevented from saving rules that include malformed regex. There may be some malformed regex in the existing rules that has to be located and fixed, but it shouldn't happen in the future.

overlordq wrote:

No, try creating an abuse filter with an invalid regex, click submit. You dont get a friendly error message, you get the internal error message and a backtrace.

matthew.britton wrote:

Looking at the backtrace, is the error actually in the exception handler rather than the regex handling code?

The backtrace in question, for those of you following at home:

MediaWiki internal error.

Original exception: exception 'AFPUserVisibleException' with message 'Error in regular expression "" at character : "Undefined index: flag"' in D:\htdocs\w\extensions\AbuseFilter\AbuseFilter.parser.php:1604
Stack trace:
#0 D:\htdocs\w\extensions\AbuseFilter\Views\AbuseFilterViewEdit.php(419): AbuseFilterParser::regexErrorHandler(8, 'Undefined index...', 'D:\htdocs\w\ext...', 419, Array)
#1 D:\htdocs\w\extensions\AbuseFilter\Views\AbuseFilterViewEdit.php(385): AbuseFilterViewEdit->buildConsequenceEditor(Object(stdClass), Array)
#2 D:\htdocs\w\extensions\AbuseFilter\Views\AbuseFilterViewEdit.php(37): AbuseFilterViewEdit->buildFilterEditor('<p>There is a s...', 'new', NULL)
#3 D:\htdocs\w\extensions\AbuseFilter\SpecialAbuseFilter.php(107): AbuseFilterViewEdit->show()
#4 D:\htdocs\w\includes\SpecialPage.php(559): SpecialAbuseFilter->execute('new')
#5 D:\htdocs\w\includes\Wiki.php(233): SpecialPage::executePath(Object(Title))
#6 D:\htdocs\w\includes\Wiki.php(62): MediaWiki->initializeSpecialCases(Object(Title), Object(OutputPage), Object(WebRequest))
#7 D:\htdocs\w\index.php(116): MediaWiki->initialize(Object(Title), NULL, Object(OutputPage), Object(User), Object(WebRequest))
#8 {main}

Exception caught inside exception handler: exception 'AFPUserVisibleException' with message 'Error in regular expression "" at character : "date() [<a href='function.date'>function.date</a>]: It is not safe to rely on the system's timezone settings. Please use the date.timezone setting, the TZ environment variable or the date_default_timezone_set() function. In case you used any of those methods and you are still getting this warning, you most likely misspelled the timezone identifier. We selected 'Europe/London' for '1.0/DST' instead"' in D:\htdocs\w\extensions\AbuseFilter\AbuseFilter.parser.php:1604
Stack trace:
#0 [internal function]: AbuseFilterParser::regexErrorHandler(2048, 'date() [<a href...', 'D:\htdocs\w\inc...', 2343, Array)
#1 D:\htdocs\w\includes\parser\Parser.php(2343): date('YmdHis', 1245089852)
#2 D:\htdocs\w\includes\parser\Parser.php(2749): Parser->getVariableValue('sitename')
#3 D:\htdocs\w\includes\parser\Preprocessor_DOM.php(959): Parser->braceSubstitution(Array, Object(PPFrame_DOM))
#4 D:\htdocs\w\includes\parser\Parser.php(2638): PPFrame_DOM->expand(Object(PPNode_DOM), 0)
#5 D:\htdocs\w\includes\parser\Parser.php(464): Parser->replaceVariables('$1 - {{SITENAME...')
#6 D:\htdocs\w\includes\parser\Parser.php(3941): Parser->preprocess('$1 - {{SITENAME...', Object(Title), Object(ParserOptions))
#7 D:\htdocs\w\includes\MessageCache.php(688): Parser->transformMsg('$1 - {{SITENAME...', Object(ParserOptions))
#8 D:\htdocs\w\includes\GlobalFunctions.php(648): MessageCache->transform('$1 - {{SITENAME...')
#9 D:\htdocs\w\includes\GlobalFunctions.php(611): wfMsgGetKey('pagetitle', true, false, true)
#10 D:\htdocs\w\includes\GlobalFunctions.php(517): wfMsgReal('pagetitle', Array, true)
#11 D:\htdocs\w\includes\OutputPage.php(357): wfMsg('pagetitle', 'Internal error')
#12 D:\htdocs\w\includes\Exception.php(148): OutputPage->setPageTitle('Internal error')
#13 D:\htdocs\w\includes\Exception.php(186): MWException->reportHTML()
#14 D:\htdocs\w\includes\Exception.php(284): MWException->report()
#15 D:\htdocs\w\includes\Exception.php(343): wfReportException(Object(AFPUserVisibleException))
#16 [internal function]: wfExceptionHandler(Object(AFPUserVisibleException))
#17 {main}

matthew.britton wrote:

Seems the cause is this in getVariableValue() in Parser.php:

2342 wfSuppressWarnings(); // E_STRICT system time bitching
2343 $localTimestamp = date( 'YmdHis', $ts );

I assume the bitching is not being sufficiently suppressed.

overlordq wrote:

Backtrace

That is unrelated to this bugreport, attached is the backtrace as evident on enwp.

Attached:

overlordq wrote:

Patch to fix error handler

The error control operator does not suppress the calling of the error handler, so the error handler needs to check error_reporting to see if it should be suppressed or not.

Attached:

Applied in r52047, thanks for the report and patch.