Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#49404 closed defect (bug) (fixed)

WP_REST_Request->parse_json_params only supports 'application/json'

Reported by: pfefferle's profile pfefferle Owned by: timothyblynjacobs's profile 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.


4 years ago

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


4 years ago

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

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

#6 @pfefferle
4 years ago

Oops, wrong account :(

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

#7 @TimothyBlynJacobs
4 years ago

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

#8 @pfefferle
4 years 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
4 years ago

No worries at all!

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


4 years ago
#10

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

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

#11 @pfefferle
4 years ago

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

#12 @pfefferle
4 years ago

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

TimothyBJacobs commented on PR #278:


4 years ago
#13

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.

pfefferle commented on PR #278:


4 years ago
#14

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.

TimothyBJacobs commented on PR #278:


4 years ago
#15

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?

pfefferle commented on PR #278:


4 years ago
#16

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

TimothyBJacobs commented on PR #278:


4 years ago
#17

I like it!

pfefferle commented on PR #278:


4 years ago
#18

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?!?

TimothyBJacobs commented on PR #278:


4 years ago
#19

That works for me!

pfefferle commented on PR #278:


4 years ago
#20

@TimothyBJacobs I added a is_json_content_type method with some tests.

TimothyBJacobs commented on PR #278:


4 years ago
#21

@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?

pfefferle commented on PR #278:


4 years ago
#22

@TimothyBJacobs oops, forgot about that!

TimothyBJacobs commented on PR #278:


4 years ago
#23

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.

pfefferle commented on PR #278:


4 years ago
#24

@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?!?

TimothyBJacobs commented on PR #278:


4 years ago
#25

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.

pfefferle commented on PR #278:


4 years ago
#26

@TimothyBJacobs like this? we should discuss the namings!

TimothyBJacobs commented on PR #278:


4 years ago
#27

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?

pfefferle commented on PR #278:


4 years ago
#28

@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) ""

}
`

TimothyBJacobs commented on PR #278:


4 years ago
#29

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

pfefferle commented on PR #278:


4 years ago
#30

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

pfefferle commented on PR #278:


4 years ago
#31

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

pfefferle commented on PR #278:


4 years ago
#32

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

TimothyBJacobs commented on PR #278:


4 years ago
#33

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.

pfefferle commented on PR #278:


4 years ago
#34

Sure!

pfefferle commented on PR #278:


4 years ago
#35

Sure!

TimothyBJacobs commented on PR #278:


4 years ago
#36

@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.

TimothyBJacobs commented on PR #278:


4 years ago
#37

@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.

pfefferle commented on PR #278:


4 years ago
#38

@TimothyBJacobs can you re-check?

TimothyBJacobs commented on PR #278:


4 years ago
#39

@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()

pfefferle commented on PR #278:


4 years ago
#40

👍

#41 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Future Release to 5.6

#42 @TimothyBlynJacobs
4 years 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.

TimothyBJacobs commented on PR #278:


4 years ago
#43

Fixed in d56906f5b0fec8cce8829bb548a8c7c7e4c64b80. Thanks for the patch @pfefferle!

Note: See TracTickets for help on using tickets.