WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 2 days 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 (23)

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


4 months ago

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


4 months ago

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

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

#6 @pfefferle
3 months ago

Oops, wrong account :(

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

#7 @TimothyBlynJacobs
2 weeks ago

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

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

No worries at all!

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


13 days ago

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

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

#11 @pfefferle
13 days ago

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

#12 @pfefferle
13 days ago

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

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

pfefferle commented on PR #278:

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

#17 @prbot
11 days ago

TimothyBJacobs commented on PR #278:

I like it!

#18 @prbot
11 days 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
11 days ago

TimothyBJacobs commented on PR #278:

That works for me!

#20 @prbot
4 days ago

pfefferle commented on PR #278:

@TimothyBJacobs I added a is_json_content_type method with some tests.

#21 @prbot
4 days 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
2 days ago

pfefferle commented on PR #278:

@TimothyBJacobs oops, forgot about that!

#23 @prbot
2 days 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.

Note: See TracTickets for help on using tickets.