Page MenuHomePhabricator

PageRestriction should handle invalid titles
Closed, ResolvedPublic3 Estimated Story Points

Description

If an invalid title name is given to the api it fails also: Exception caught: "Call to a member function getArticleID() on null" on PageRestriction.php(114)
Invalid title is "In<valid"

Try:
https://en.wikipedia.beta.wmflabs.org/wiki/Special:ApiSandbox#action=block&format=json&user=Drwpb&allowusertalk=1&partial=1&pagerestrictions=nonexistentpage%7Cin%3Cvalid

There is no error handling in PageRestriction::newFromTitle and perhaps there should be.

$ ag "PageRestriction::newFromTitle"
includes/specials/SpecialBlock.php
829:                                    $pageRestrictions[] = PageRestriction::newFromTitle( $title );

includes/api/ApiBlock.php
135:                            $pageRestrictions[] = PageRestriction::newFromTitle( $title );

Some other things to think about (from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/676290/4/includes/block/BlockUser.php#441):

Perhaps we should keep this scoped to nonexistent titles and file the issue with invalid titles separately (thanks for raising it, Umherirrender). We might well want to make changes to PageRestriction::newFromTitle... We might even want to move creating the Restriction objects to BlockUser rather than repeating this in the API and the Special Page...

Event Timeline

Passing along an invalid page returns an error like this:

{
    "error": {
        "code": "internal_api_error_Error",
        "info": "[d6a020e0ea346e5dc13507ec] Exception caught: Call to a member function getArticleID() on null",
        "errorclass": "Error",
        "*": "Error at /var/www/html/w/includes/block/Restriction/PageRestriction.php(114)\nfrom /var/www/html/w/includes/block/Restriction/PageRestriction.php(114)\n#0 /var/www/html/w/includes/api/ApiBlock.php(143): MediaWiki\\Block\\Restriction\\PageRestriction::newFromTitle(NULL)\n#1 /var/www/html/w/includes/api/ApiMain.php(1721): ApiBlock->execute()\n#2 /var/www/html/w/includes/api/ApiMain.php(702): ApiMain->executeAction()\n#3 /var/www/html/w/includes/api/ApiMain.php(673): ApiMain->executeActionWithErrorHandling()\n#4 /var/www/html/w/api.php(90): ApiMain->execute()\n#5 /var/www/html/w/api.php(45): wfApiMain()\n#6 {main}"
    },
    "servedby": "4e9a4437aea6"
}

You can test this locally w/the API sandbox: Special:ApiSandbox#action=block&format=json&user=Blocked%20User%201&allowusertalk=1&partial=1&pagerestrictions=nonexistentpage%7Cin%3Cvalid

The check can't handle the input In<valid but since we don't catch it, it gets let through and then causes errors.

Ideally, it'd be something cleaner like this:

{
    "error": {
        "code": "cant-block-nonexistent-page",
        "info": "You cannot block a user from Nonexistentpage because the page does not exist",
        "*": "See http://localhost:8080/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at &lt;https://lists.wikimedia.org/mailman/listinfo/mediawiki-api-announce&gt; for notice of API deprecations and breaking changes."
    },
    "servedby": "4e9a4437aea6"
}

Special:ApiSandbox#action=block&format=json&user=Blocked%20User%201&allowusertalk=1&partial=1&pagerestrictions=nonexistentpage

Because we check for and catch nonexistent pages.

The “internal api error” is “page restrictions don’t handle invalid inputs” and we want the error to be “you put in an invalid input (and we caught it).” The primary goal of this ticket is “return an error after we catch an invalid input” and the bonus part of this ticket is “determine where that catch should go and if it can be DRYed up a bit.”

so we're trying to get the error to be more specific and not stop the error from generating?

Yes. An error is expected but we are being proactive about it and explicitly handling it so we can provide a more user-friendly error.

fatal errors like "internal_api_error" are also logged and if happen to often there are create alarms to some people

Change 702115 had a related patch set uploaded (by Wikitrent; author: Wikitrent):

[mediawiki/core@master] WIP: Handle PageRestriction error on invalid title

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

Per the standup thread, Trent has been able to get https://gerrit.wikimedia.org/r/c/mediawiki/core/+/702115 to show a better API error from ApiBlock but has not been able to replicate the issue with SpecialBlock. He suggested splitting it and I think it could be a good idea. Should we make the error a specialblock error a spike, since it seem to need some looking into? @Niharika @STran

The special page part should/could only happen for non-js, because the js validation would catch the most things. But not sure if that could be bypassed.

Change 702115 abandoned by Wikitrent:

[mediawiki/core@master] Handle PageRestriction error on invalid title

Reason:

Better solution available on other branch

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

Change 708096 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/core@master] api: Handle invalid/non-existent titles in ApiBlock

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

Change 708096 merged by jenkins-bot:

[mediawiki/core@master] api: Handle invalid/non-existent titles in ApiBlock

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

The testing I did confirmed that there is error handling message when there is invalid page title. And also confirm there is no error message for valid page title message.
Below is where the API request with invalid title page is generated:

Screen Shot 2021-08-04 at 3.46.44 PM.png (1×2 px, 677 KB)

Below shows the copy of cURL message ran on the terminal, the error handling message for invalid title page is highlighted. Note: per dev we are unable to test this feature using the API sandbox since the invalid error message is blocked already upon api request.
Screen Shot 2021-08-04 at 3.47.35 PM.png (1×1 px, 464 KB)