WordPress.org

Make WordPress Core

Opened 9 months ago

Last modified 3 weeks ago

#49404 new defect (bug)

WP_REST_Request->parse_json_params only supports 'application/json'

Reported by: pfefferle Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: good-first-bug has-patch has-unit-tests
Focuses: rest-api Cc:

Description

The parse_json_params function only supports application/json to parse the body as JSON, that means every subtype like application/ld+json or application/activity+json will be ignored. It is also possible to add additional data like application/json; profile="https://www.w3.org/ns/activitystreams" that will be also ignored by the current implementation.

I would recomment to use a more generic check like:

preg_match( '/application\/(\w+\+)?json.*/i', $content_type )

and/or a filter to overwrite the check.

Change History (35)

This ticket was mentioned in Slack in #core-restapi by pfefferle. View the logs.


9 months ago

#2 @pfefferle
8 months ago

JSON Content-(Sub)Type examples:

ActivityPub https://www.w3.org/TR/activitypub/

JF2Feed https://www.w3.org/TR/jf2/

  • application/jf2feed+json

JSON Web Signature (JWS) https://tools.ietf.org/html/rfc7515

  • application/jose+json

WebFinger https://tools.ietf.org/html/rfc7033

  • application/jrd+json

Here is the complete list: https://www.iana.org/assignments/media-types/media-types.xhtml#application

This ticket was mentioned in Slack in #core by noisysocks. View the logs.


8 months ago

#4 @TimothyBlynJacobs
8 months ago

  • Keywords needs-patch needs-unit-tests good-first-bug added
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 4.4

Thanks for the ticket @pfefferle!

I agree this would be a good change to make, are you interested in working on a patch for this?

#5 @ionosbot
8 months ago

Sure, I will give it a try and prepare a patch.

#6 @pfefferle
8 months ago

Oops, wrong account :(

Sure, I will give it a try and prepare a patch.

#7 @TimothyBlynJacobs
5 months ago

Hey @pfefferle, just checking in. Are you still interested in working on this? Have you run into any issues?

#8 @pfefferle
5 months ago

Hey @TimothyBlynJacobs sure, I am still interested in working on this. Sorry for beeing so passive, had some busy weeks! I plan to submit a diff, the next few days.

#9 @TimothyBlynJacobs
5 months ago

No worries at all!

This ticket was mentioned in PR #278 on WordPress/wordpress-develop by pfefferle.


5 months ago

Add support for alternate JSON Content-Types like application/ld+json.

Trac ticket: https://core.trac.wordpress.org/ticket/49404

#11 @pfefferle
5 months ago

@TimothyBlynJacobs I commited the changes via GitHub (including some tests).

#12 @pfefferle
5 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

#13 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

Thanks for the PR @pfefferle!

Why is get_parameter_order using a different check from parse_json_params?

We'll probably also want to update wp_is_json_request.

#14 @prbot
5 months ago

pfefferle commented on PR #278:

I thought it might be more performat to only check if there is JSON in the content type, because otherwise preg_match would be called twice. But sure, I can use the preg_match in both cases.

#15 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

My concern about making the parameter order check looser is that we'd be saying this request contains JSON data when really it doesn't.

Maybe introduce a protected is_json_request() method on the request object and cache the results of the regex check?

#16 @prbot
5 months ago

pfefferle commented on PR #278:

@TimothyBJacobs what do you think about the preg_match in general?

#17 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

I like it!

#18 @prbot
5 months ago

pfefferle commented on PR #278:

I like the is_json_request() idea, but I think is_json_content_type() would be a better match, because it only checks the content_type and the content body still might be broken/non JSON?!?

#19 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

That works for me!

#20 @prbot
5 months ago

pfefferle commented on PR #278:

@TimothyBJacobs I added a is_json_content_type method with some tests.

#21 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

@pfefferle can we cache that value as long as the content-type header hasn't changed? That way we don't have to worry as much about the impact of the regex check?

#22 @prbot
5 months ago

pfefferle commented on PR #278:

@TimothyBJacobs oops, forgot about that!

#23 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

We also have to worry about add_header and remove_header. It may be better to build into the cache the value of the content-type header when it evaluated it.

#24 @prbot
5 months ago

pfefferle commented on PR #278:

@TimothyBJacobs do you think it is worth it? I do think the complete "caching" and "invalidate caches" is very error-prone and it is "only" a preg_match in the end?!?

#25 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

You're definitely right that is error prone. If the preg_match would only occur a couple of times in a request, I think it wouldn't be too bad. But AFAICT, this would result in a preg_match call for every single get or set of a parameter which could add up.

#26 @prbot
5 months ago

pfefferle commented on PR #278:

@TimothyBJacobs like this? we should discuss the namings!

#27 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

I like that! Let's get some tests on the different ways we can modify the header.

we should discuss the namings!

Namings make sense to me! Do you have any concerns about them?

#28 @prbot
5 months ago

pfefferle commented on PR #278:

@TimothyBJacobs while writing tests, I think I found a bug. Content-Type should always contain a single type, even if you use $request->add_header();, but it works like the Accept header and adds them to a list.

`PHP
$request->add_header( 'Content-Type', 'application/json' );
$request->add_header( 'Content-Type', 'application/activity+json' );
`

Result for $request->get_header( 'Content-Type' ) is application/json,application/activity+json

Result for $request->get_content_type() is:

`
array(4) {

value?=>
string(78) "application/json,application/activity+json"
type?=>
string(11) "application"
subtype?=>
string(66) "json,application/activity+json"
parameters?=>
string(0) ""

}
`

#29 @prbot
5 months ago

TimothyBJacobs commented on PR #278:

I think we should consider that in a separate ticket. For now let's just keep operating on the first header in the list.

#30 @prbot
5 months ago

pfefferle commented on PR #278:

I only mentioned it, because it broke the test for add_header but I will rewrite them.

#31 @prbot
5 months ago

pfefferle commented on PR #278:

wp_is_json_request should use the same preg_match pattern https://wordpress.slack.com/archives/C02RQC26G/p1591294944063300

#32 @prbot
3 weeks ago

pfefferle commented on PR #278:

@TimothyBJacobs I added tests. Can you re-review?

#33 @prbot
3 weeks ago

TimothyBJacobs commented on PR #278:

Do you want to update wp_is_json_request in this PR as well? Would probably be good to keep them synced like you mentioned.

#34 @prbot
3 weeks ago

pfefferle commented on PR #278:

Sure!

#35 @prbot
3 weeks ago

pfefferle commented on PR #278:

Sure!

Note: See TracTickets for help on using tickets.