Page MenuHomePhabricator

JsonBodyValidator does not validate the parameter types
Open, Needs TriagePublic

Description

As a TODO comment notes:

// TODO: use a ParamValidator to check field value, etc!

Currently, JsonBodyValidator does not perform any type check, which means that handlers could break horribly if they don't do the validation themselves (which they shouldn't do!).

Event Timeline

We (WB Product Platform Team) recently looked at JsonBodyValidator for the Wikibase REST API and ended up creating a subclass in Wikibase to at least do basic type validation for primitive types. It would of course make sense for something like this to be upstreamed, but we weren't sure about the exact response format (keys and message), and whether this limited version is useful there at all.

We saw the comment in the code regarding ParamValidator, but it doesn't look like it can easily be used there. ParamValidator and the TypeDef objects seem to assume their input values to be strings, which makes sense for path params and query params, but not for JSON.

If someone finds bits of this TypeValidatingJsonBodyValidator in Wikibase useful, feel free to copy those over, or poke someone from our team to create a patch for core! :)

Change 809322 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Rest: ensure there are no extraneous params in JSON bodies

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

The patch above does something, although it's not enough for this task. This seemed easier than validating types, though, so I thought I'd fix it.

Change 809567 had a related patch set uploaded (by Ollie Shotton; author: Ollie Shotton):

[mediawiki/extensions/Wikibase@master] REST: Extraneous params not allowed in JSON bodies

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

One more thing to note: JsonBodyValidator also does not support/check PARAM_ISMULTI, which I assume is in scope for this task.

kostajh added a subscriber: kostajh.

The patch above does something, although it's not enough for this task. This seemed easier than validating types, though, so I thought I'd fix it.

It looks like an improvement to me; I've +2'ed it but it's depending on a Wikibase patch which I +1'ed.

I was surprised that JsonBodyValidator is not actually validating; if we're not going to fix this issue anytime soon, we should probably audit all uses of getBodyValidator to make sure that they don't assume that the fields are validated in ways that are unsafe after calling $this->getValidatedBody();

Change 809322 merged by jenkins-bot:

[mediawiki/core@master] Rest: ensure there are no extraneous params in JSON bodies

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

I was surprised that JsonBodyValidator is not actually validating; if we're not going to fix this issue anytime soon, we should probably audit all uses of getBodyValidator to make sure that they don't assume that the fields are validated in ways that are unsafe after calling $this->getValidatedBody();

I think this should really be fixed, considering that REST APIs are being used in more and more places, and that JSON is pretty much the only format currently available. But then of course I'm not very familiar with the REST API framework, and I also don't know how this task fits into the API team's plans.

Krinkle added subscribers: BPirkle, Krinkle.

@BPirkle I'm boldly moving this back to the inbox for triage given that it was moved to the radar back in October before the current reorg. This seems outside the Platform team's scope, but let us know if you find something you need help with.