Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#36419 closed defect (bug) (fixed)

Request data is inconsistently slashed in REST API callbacks

Reported by: joehoyle's profile joehoyle Owned by: markjaquith's profile markjaquith
Milestone: 4.5 Priority: highest omg bbq
Severity: blocker Version: 4.4
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

Requests made to the REST API inconsistently have slashes added to the data passed into a callback based off whether the data came from $_POST, $_GET, JSON Body, or URL Params.

This is because WordPress adds slashes to the superglobals, and we don't remove those in the REST API, which we probably should.

This will potentially break Backwards Compatibility, however because data can be passed in a variety of ways into the REST API, this means the REST API callbacks can not presume if the data is slashed or unslashed.

@rmccue will be filling out much more detail on this issue, and I'll be writing a patch to unslash the superglobals before they are passed to the request object.

Attachments (1)

36419.diff (5.8 KB) - added by joehoyle 9 years ago.

Download all attachments as: .zip

Change History (21)

@joehoyle
9 years ago

#1 @joehoyle
9 years ago

  • Keywords has-patch has-unit-tests dev-feedback added
  • Severity changed from critical to blocker

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


9 years ago

#3 @rmccue
9 years ago

  • Owner set to markjaquith
  • Status changed from new to reviewing

#4 follow-up: @jorbin
9 years ago

This absolutely requires a make core post (and an update to the field guide) when it goes in.

#5 in reply to: ↑ 4 @kirasong
9 years ago

Replying to jorbin:

This absolutely requires a make core post (and an update to the field guide) when it goes in.

+1. It should not happen if there is not an immediate make post/field guide update.

@markjaquith: 36419.diff is available to review.

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


9 years ago

#8 @dd32
9 years ago

Looks good to me.

The approach being taken here looks correct to me. Although other handlers in WordPress (Ajax handlers for example) force slashed data, standardising on unslashed data for the Rest API is appropriate.

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


9 years ago

#10 @pento
9 years ago

I agree with @dd32 that the approach is correct, but I'm so wildly unexcited about putting in a breaking change like this during RC.

Given the lack of bug reports from outside sources, is there a need for such and urgent fix? This kind of subtle breaking change is much better to commit early in a release cycle for plenty of soak time, not a week before the final release.

The draft post on make/core is good. If we do decide go ahead with this, we should email a link to plugin authors who use the REST API, to ensure they see it and have a chance to check their work before release.

#11 follow-up: @dd32
9 years ago

but I'm so wildly unexcited about putting in a breaking change like this during RC.

I'm not wild about it either, but figure we're going to have to fix it sooner or later, and I'd rather just rip the bandaid off sooner than later.

The inconsistency comes in that the slashing the application gets depends 100% on where the data came from - JSON blob POST'd or POST params - and the endpoint doesn't know which it is (I mean, there's hacky ways to detect it I'm sure). The client decides if the application gets unslashed data (JSON blob) or slashed data (POST params).

I feel that just fixing it to be unslashed is a legit bugfix that will fix more than it'll do harm.
Leaving it for a soak will mean that we'll maintain the current inconsistent and buggy approach until 4.6 is released (and 4.4.x/4.5.x won't get fixed).

#12 in reply to: ↑ 11 @pento
9 years ago

  • Keywords commit added; dev-feedback removed

Replying to dd32:

I'm not wild about it either, but figure we're going to have to fix it sooner or later, and I'd rather just rip the bandaid off sooner than later.

Ugh. I agree, it's only going to get worse to fix if we wait. It seems that the worst case is that we'd need to roll out a very quick 4.5.1 release, which isn't so bad.

+1 to commit, and may god have mercy on our souls.

#13 @jorbin
9 years ago

Love the idea of emailing all plugin authors that this affects. Perhaps @ipstenu and @coffee2code could help with that?

I'm not sure I love the idea of backporting this. Backporting backwards incompatible changes isn't something that should be done except in security situations.

#14 @dd32
9 years ago

I'm not sure I love the idea of backporting this. Backporting backwards incompatible changes isn't something that should be done except in security situations.

I'm not too worried about this as we do backport major bugs often. This is the sort of thing that would be done in a .1 or .2 without question, of course with 4.5's imminent release 4.4.x is now heading to security-fixes only area which is where this becomes a bit more of a grey area.
The only reason I support backporting it, is because it's a fairly big change that anyone who is affected by it would be far more happy to just change it for all versions, rather than version-detect.

#15 @mordauk
9 years ago

For what it's worth, one of my plugins is affected by this and I support this change.

#16 @Ipstenu
9 years ago

I can get a list of everyone using the json API, but I don't know if there's a way to isolate them further.

Here's the current list of 44 (checking for register_rest_route):

acf-to-rest-api, acf-to-wp-api, acf-to-wp-rest-api, admin-generator-advanced, application-passwords, be-rest-endpoints, custom-contact-forms, dashboard-directory-size, easy-student-results, event-espresso-decaf, hugh, invitations-for-slack, json-rest-api-subscriptions, jwt-authentication-for-wp-rest-api, metronet-profile-picture, nugget-by-ingot, oembed-api, prayers, projecthuddle-trello-integration, prospect, qqworld-passport, react, rest-api, rest-api-link-manager, rest-api-meta-endpoints, rest-json, rest-routes, restful-hello-dolly, searchwp-api, sortable-posts, sportspress, store-locator-le, tabulate, version-dashboard, wp-api-categoriestags, wp-api-menus, wp-dev-kit, wp-rest-api-all-terms, wp-rest-api-log, wp-rest-api-post-type-taxonomies, wp-rest-api-sidebars, wp-rest-api-v2-menus, wprestapiextensions, yarakuzen

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


9 years ago

#18 @coffee2code
9 years ago

Happy to handle an emailing to affected plugin authors about this. What is needed:

  1. Template for email to be sent.
  2. List of plugins to email (if other than the list Ipstenu produced). Either a specific list, or at least the specific criteria that can be used for a scan to identify affected plugins.
  3. Secondary sign-off from someone associated with the release (lead, committer, etc) who agrees the emailing is justified and the email template is acceptable.
  4. Desired send date.

#19 @markjaquith
9 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 37163:

REST API: Deliver parameters unadulterated instead of slashed.

We goofed, and parameters accessed through the REST API's methods
were slashed (inconsistently, even). This unslashes the data, so
you get the un-messed-with data that was sent.

Props joehoyle.
Fixes #36419.

Note: See TracTickets for help on using tickets.