Page MenuHomePhabricator

Parsoid transformation API in rest.php unusable with mw.Rest
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):
On browser debug console:

const rest = new mw.Rest();
await rest.post("/v1/transform/wikitext/to/html", { wikitext: "{{citation needed}}" }).catch(console.error)

What happens?:
An http JQuery error is thrown, with the following exception:

SyntaxError: Unexpected token '<', "<!DOCTYPE "... is not valid JSON
    at parse (<anonymous>)
    at ajaxConvert (jquery.js:9099:19)
    at done (jquery.js:9577:15)
    at XMLHttpRequest.<anonymous> (jquery.js:9888:9)

JQuery attempts to convert the response (text/html) to JSON.

What should have happened instead?:
No error should have been thrown, a string containing the response text should have been returned.

Software version: 1.42.0-wmf.7 (6765518)

Other information:
This is probably more of a JavaScript issue, since mediawiki.api/rest.js:118 currently forces dataType: 'json'. Setting dataType to nothing allows JQuery to intelligently guess the data type and automatically return the appropriate data type (e.g. returning JSON if the Content-Type matches /\bjson\b/, exact mechanism for this found here+here). Omitting the dataType will probably be fine, as long as the REST API always sends the correct Content-Type response header, but this will affect everything that consumes from the REST API using mw.Rest and I don't have any definitive evidence that Content-Types are always accurate so I'll leave the choice of what to do here up to someone who can make a more-informed decision.

Event Timeline

cc Parsoid, since this seems to be an issue specific to the newly-introduced endpoints in T350661.

Change 979440 had a related patch set uploaded (by Chlod Alejandro; author: Chlod Alejandro):

[mediawiki/core@master] Remove dataType from mw.Rest AJAX requests

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

Change 979441 had a related patch set uploaded (by Chlod Alejandro; author: Chlod Alejandro):

[mediawiki/core@master] Add Content-Type header checks to REST API tests

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

I've submitted https://gerrit.wikimedia.org/r/979440 to remove the dataType field in mw.Rest AJAX calls. This does, however, mean that any REST endpoint that returns an improper Content-Type header may cause JQuery to return a string for a JSON response. I'm working on https://gerrit.wikimedia.org/r/979441 to add Content-Type checking to REST API tests, so that this could be caught before it has any impact on the client. I'll take it out of WIP when I'm finished.

I've also noticed this problem when playing with the Parsoid REST APIs, and I had no idea the fix would look so simple, thank you!

I think we can rely on accurate Content-Type headers. Setting them wrong causes fairly obvious issues (and can cause security issues, if non-HTML content is served as HTML), so we're careful about doing that. It's a good idea to add tests for it though, and it's reassuring that all of your new tests apparently pass with no code changes.

Change 979440 merged by jenkins-bot:

[mediawiki/core@master] Remove dataType from mw.Rest AJAX requests

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

Change 979441 merged by jenkins-bot:

[mediawiki/core@master] Add Content-Type header checks to REST API tests

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

matmarex assigned this task to Chlod.

Thank you for the patches!

Thank you for reviewing (and getting them additional reviews) as well, @matmarex! :)