WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 8 months ago

Last modified 8 months ago

#49404 closed defect (bug) (fixed)

WP_REST_Request->parse_json_params only supports 'application/json'

Reported by: pfefferle Owned by: TimothyBlynJacobs
Milestone: 5.6 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 (43)

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


16 months ago

#2 @pfefferle
16 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.


16 months ago

#4 @TimothyBlynJacobs
16 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
16 months ago

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

#6 @pfefferle
16 months ago

Oops, wrong account :(

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

#7 @TimothyBlynJacobs
13 months ago

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

#8 @pfefferle
13 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
13 months ago

No worries at all!

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


13 months ago

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

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

#11 @pfefferle
13 months ago

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

#12 @pfefferle
13 months ago

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

#13 @prbot
13 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
13 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
13 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
13 months ago

pfefferle commented on PR #278:

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

#17 @prbot
13 months ago

TimothyBJacobs commented on PR #278:

I like it!

#18 @prbot
13 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
13 months ago

TimothyBJacobs commented on PR #278:

That works for me!

#20 @prbot
13 months ago

pfefferle commented on PR #278:

@TimothyBJacobs I added a is_json_content_type method with some tests.

#21 @prbot
13 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
13 months ago

pfefferle commented on PR #278:

@TimothyBJacobs oops, forgot about that!

#23 @prbot
13 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
13 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
13 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
13 months ago

pfefferle commented on PR #278:

@TimothyBJacobs like this? we should discuss the namings!

#27 @prbot
13 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
13 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
13 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
13 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
13 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
9 months ago

pfefferle commented on PR #278:

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

#33 @prbot
9 months 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
9 months ago

pfefferle commented on PR #278:

Sure!

#35 @prbot
9 months ago

pfefferle commented on PR #278:

Sure!

#36 @prbot
8 months ago

TimothyBJacobs commented on PR #278:

@pfefferle following up on this. If we want to include it in 5.6 I'd like to have a patch in the next few days. Ideally by Monday.

#37 @prbot
8 months ago

TimothyBJacobs commented on PR #278:

@pfefferle What about localizing the cache to wp_is_json_content_type() with a static variable? That way we can simplify the changes to WP_REST_Request, and would make caching wp_is_json_request() simple as well.

#38 @prbot
8 months ago

pfefferle commented on PR #278:

@TimothyBJacobs can you re-check?

#39 @prbot
8 months ago

TimothyBJacobs commented on PR #278:

@pfefferle This is looking good, thanks! To clarify, since wp_is_json_request can now be used by multiple consumers, it should cache any input, not just the last one. So a static $is_json[ $header ] = regex_check()

#40 @prbot
8 months ago

pfefferle commented on PR #278:

👍

#41 @TimothyBlynJacobs
8 months ago

  • Milestone changed from Future Release to 5.6

#42 @TimothyBlynJacobs
8 months ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 49329:

REST API: Support a broader range of JSON media types.

Previously, we only supported application/json which prevented using subtypes like application/activity+json. This allows for the REST API to json_decode the body of requests using a JSON subtype Content-Type. Additionally, wp_die() now properly sends the error as JSON when a JSON subtype is specified in the Accept header.

Props pfefferle.
Fixes #49404.

#43 @prbot
8 months ago

TimothyBJacobs commented on PR #278:

Fixed in d56906f5b0fec8cce8829bb548a8c7c7e4c64b80. Thanks for the patch @pfefferle!

Note: See TracTickets for help on using tickets.