Page MenuHomePhabricator

Fatal MWNamespace exception on Special:Contributions when given namespace is a negative number
Closed, ResolvedPublic

Description

reproduce:

https://mediawiki.org/w/index.php?namespace=-1%27&month=-1&limit=50&title=Special:Contributions&contribs=newbie&contribs=user&target=217.23.7.140&nsInvert=1&associated=1&tagfilter=1&topOnly=1&newOnly=1&hideMinor=1&year=2016

MWNamespace::getAssociated does not make any sense for given namespace -1

#0 /srv/mediawiki/php-1.29.0-wmf.2/includes/MWNamespace.php(142): MWNamespace::isMethodValidFor(integer, string)
#1 /srv/mediawiki/php-1.29.0-wmf.2/includes/specials/pagers/ContribsPager.php(274): MWNamespace::getAssociated(integer)
#2 /srv/mediawiki/php-1.29.0-wmf.2/includes/specials/pagers/ContribsPager.php(159): ContribsPager->getNamespaceCond()
#3 /srv/mediawiki/php-1.29.0-wmf.2/includes/pager/IndexPager.php(378): ContribsPager->getQueryInfo()
#4 /srv/mediawiki/php-1.29.0-wmf.2/includes/specials/pagers/ContribsPager.php(101): IndexPager->buildQueryInfo(string, integer, boolean)
#5 /srv/mediawiki/php-1.29.0-wmf.2/includes/pager/IndexPager.php(214): ContribsPager->reallyDoQuery(string, integer, boolean)
#6 /srv/mediawiki/php-1.29.0-wmf.2/includes/pager/IndexPager.php(554): IndexPager->doQuery()
#7 /srv/mediawiki/php-1.29.0-wmf.2/includes/specials/SpecialContributions.php(203): IndexPager->getNumRows()
#8 /srv/mediawiki/php-1.29.0-wmf.2/includes/specialpage/SpecialPage.php(522): SpecialContributions->execute(NULL)
#9 /srv/mediawiki/php-1.29.0-wmf.2/includes/specialpage/SpecialPageFactory.php(584): SpecialPage->run(NULL)
#10 /srv/mediawiki/php-1.29.0-wmf.2/includes/MediaWiki.php(283): SpecialPageFactory::executePath(Title, RequestContext)
#11 /srv/mediawiki/php-1.29.0-wmf.2/includes/MediaWiki.php(861): MediaWiki->performRequest()
#12 /srv/mediawiki/php-1.29.0-wmf.2/includes/MediaWiki.php(522): MediaWiki->main()
#13 /srv/mediawiki/php-1.29.0-wmf.2/index.php(43): MediaWiki->run()
#14 /srv/mediawiki/w/index.php(3): include(string)
#15 {main}

Event Timeline

Matanya created this task.Nov 9 2016, 11:14 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 9 2016, 11:14 AM
Aklapper renamed this task from mediawiki fatals if namespace is negetive to Mediawiki fatals if namespace is negative.Nov 9 2016, 12:31 PM
hashar updated the task description. (Show Details)Nov 9 2016, 12:48 PM
hashar updated the task description. (Show Details)
hashar triaged this task as Low priority.Nov 9 2016, 12:54 PM
hashar added a subscriber: hashar.

I added that exception years ago. It is thrown when attempting to get the talk or content namespace associated with respectively a content or talk namespace.

That does not make sense on the special namespaces NS_SPECIAL -1 or NS_MEDIA -2 hence the exception.

I am not sure how one would end up with a link listing contributions of someone to special pages. It does not make any sense either. Maybe when forging the URL, the namespace should be validated. Regardless the link would come from Special:Contributions which has a list of namespaces to pick from and that does not list either Special or Media namespaces.

Krinkle renamed this task from Mediawiki fatals if namespace is negative to Fatal MWNamespace exception on Special:Contributions when given namespace is a negative number.Oct 16 2018, 10:58 PM
Krinkle raised the priority of this task from Low to High.
Krinkle added a subscriber: Krinkle.

Raising priority because unlike most cases of "this is broken and it isn't meant to work" (where by it should still be fixed to not spam the logs, but at least it's not going to make a difference for uses), unlike those, fatal exceptions are particularly painful due to them bypassing caching, and triggering the highest severity of alarms that we monitor ("mw fatals").

Moments ago a stampede of about 10,000 requests hit the app servers using this particular URL scheme, which typically trips the "MediaWiki fatal exceptions" and "Varnish HTTP 5xx" operational alarms.

MWNamespace::getAssociated does not make any sense for given namespace -8673

#0 /srv/mediawiki/php-1.32.0-wmf.24/includes/MWNamespace.php(163): MWNamespace::isMethodValidFor(integer, string)
#1 /srv/mediawiki/php-1.32.0-wmf.24/includes/specials/pagers/ContribsPager.php(297): MWNamespace::getAssociated(integer)
#2 /srv/mediawiki/php-1.32.0-wmf.24/includes/specials/pagers/ContribsPager.php(253): ContribsPager->getNamespaceCond()
#3 /srv/mediawiki/php-1.32.0-wmf.24/includes/pager/IndexPager.php(382): ContribsPager->getQueryInfo()
#4 /srv/mediawiki/php-1.32.0-wmf.24/includes/pager/RangeChronologicalPager.php(104): IndexPager->buildQueryInfo(string, integer, boolean)
#5 /srv/mediawiki/php-1.32.0-wmf.24/includes/specials/pagers/ContribsPager.php(123): RangeChronologicalPager->buildQueryInfo(string, integer, boolean)
#6 /srv/mediawiki/php-1.32.0-wmf.24/includes/pager/IndexPager.php(226): ContribsPager->reallyDoQuery(string, integer, boolean)
#7 /srv/mediawiki/php-1.32.0-wmf.24/includes/pager/IndexPager.php(558): IndexPager->doQuery()
#8 /srv/mediawiki/php-1.32.0-wmf.24/includes/specials/SpecialContributions.php(226): IndexPager->getNumRows()
#9 /srv/mediawiki/php-1.32.0-wmf.24/includes/specialpage/SpecialPage.php(569): SpecialContributions->execute(NULL)

Example: https://bs.wikipedia.org/w/index.php?title=Posebno%3ADoprinos&contribs=newbie&namespace=-8673%29%2&associated=1

Question: How should this exception be handled then if we don't want it to be thrown? Which mechanism should we use to handle such cases? Maybe display a nice message on the page saying one doesn't have the permission to view the page or maybe redirect to a different page or ?

I'm not a product owner or maintainer of this component, but personally I think it might make sense to validate this parameter before using it, and if it is invalid, treat it as if that parameter was not set. That means ?contribs=user&target=Example&namespace=-9897 would lead to the same result as for ?contribs=user&target=Example (which means, show all results).

That is fairly common and would be the easiest way to implement it. If more time can be spend on a solution, even better would be to display no results, with a message indicating this parameter is invalid. In order for this error to be better than the current fatal error, the useful error needs to be displayed above the form - the form should still be there. This means the namespaces selector needs to have a value selected, and it cannot be the invalid value. So probably we'd select the default there as well.

End result: Producing a corrupted link, leads to a useful error, after which you either see the default results already, or you can submit the form and get the default results then.

Seems calls to MWNamespace::isMethodValidFor() usually originate from QueryPage::reallyDoQuery() (which process parameters to craft the SQL request). We might want to catch potential exception and show some error page instead:

includes/specialpage/QueryPage.php
/**
 * Run the query and return the result
 * @param int|bool $limit Numerical limit or false for no limit
 * @param int|bool $offset Numerical offset or false for no offset
 * @return IResultWrapper
 * @since 1.18
 */
public function reallyDoQuery( $limit, $offset = false ) {
    $fname = static::class . '::reallyDoQuery';
    $dbr = $this->getRecacheDB();
    try {
        $query = $this->getQueryInfo();
        $order = $this->getOrderFields();
    } catch( MWException $ex ) {
        
    }
    ...

Or maybe generalize in final SpecialPage::run

D3r1ck01 added a comment.EditedFeb 16 2019, 11:02 PM

As of 1.33.0-alpha (333eaea), this has disappeared. The page no longer throws a fatal error, processes the request and no result is returned with the message/result No changes were found matching these criteria. Also, below are the following notices;

  1. No fatal error is thrown.
  2. Namespace dropdown defaults to "all".
  3. The form is still available to make another request.

I think this is good and addresses some of the points @Krinkle highlighted above though in a slightly different manner. In that case, @Krinkle, do you think there is more to be added here or if this is good (at this level), can this be resolved?

Notes

Related code that addresses this issue is in ContribsPager::getNamespaceCond() called in getQueryInfo() method which is used to build the query conditions;

$queryInfo['conds'] = array_merge( $queryInfo['conds'], $this->getNamespaceCond() );

@D3r1ck01 Are you sure you followed the same steps as in this report?

The following two both still produce a fatal error:

D3r1ck01 added a comment.EditedFeb 17 2019, 8:40 AM

@Krinkle, I followed the steps but seems I missed something and I'll like to exactly explain here what the issue is as the description above is not that specific (looking at the links) as per what is going wrong or I didn't quite catch it on first few reads.

The problem only occurs when the following is true;

  • &associated=1 (the associated checkbox is checked)
  • &namespace=<any negative number> (not only -1 or -2)

Everything works well for different scenarios. So I think that is where the problem is occurring and an exception thrown which reads (for namespace=-1 case);

MWNamespace::getAssociated does not make any sense for given namespace -1

So it's trying to get an associated namespace for a namespace that is invalid but if "namespace" is set to a negative number without "associated", everything works fine.

Sorry for missing that aspect of the problem! The problem indeed does exist.

Change 491051 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/core@master] specials: Fix fatal MWNamespace exception on Special:Contributions

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

@Krinkle, I've walked through the problem and attempted to solve it using the second approach you suggested here: T150324#4897404, it indeed needed sometime to get the solution through. I've tagged you for review, thanks!

D3r1ck01 claimed this task.Feb 18 2019, 2:24 PM
D3r1ck01 added a project: User-D3r1ck01.
D3r1ck01 moved this task from Backlog to Reviewed/Resolved on the User-D3r1ck01 board.

Change 491051 merged by jenkins-bot:
[mediawiki/core@master] specials: Fix fatal MWNamespace exception on Special:Contributions

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

Change 491424 had a related patch set uploaded (by D3r1ck01; owner: Derick Alangi):
[mediawiki/core@master] specials: Follow-up on I525d305a4dabb040110894d3230

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

Change 491424 merged by jenkins-bot:
[mediawiki/core@master] Follow-up 77276ce: Clarify i18n message key/values and div class

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

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:11 PM